Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flatcar integration #53

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Flatcar integration #53

merged 1 commit into from
Oct 26, 2023

Conversation

heytrav
Copy link
Contributor

@heytrav heytrav commented Jul 5, 2023

  • Modify openstack-cluster templates to integrate ignition components for Flatcar
  • Smaller changes related to addon defaults

@heytrav heytrav requested a review from mkjpryor as a code owner July 5, 2023 09:37
@heytrav heytrav force-pushed the flatcar-integration branch 2 times, most recently from 095f920 to c48a720 Compare July 5, 2023 23:09
@heytrav heytrav force-pushed the flatcar-integration branch 4 times, most recently from ff7ef91 to 7b946f3 Compare September 13, 2023 23:20
@mkjpryor
Copy link
Collaborator

I don't like how Flatcar feels like a bolt-on here. If we are going to support it, I feel we should refactor the charts so that there is an OS abstraction that Ubuntu and Flatcar both use.

@mkjpryor
Copy link
Collaborator

I think there is a nice way to do it by utilising template definitions and passing around the name of a template to use, but I haven't got it fully figured out yet.

@heytrav
Copy link
Contributor Author

heytrav commented Sep 14, 2023

I think there is a nice way to do it by utilising template definitions and passing around the name of a template to use, but I haven't got it fully figured out yet.

I wonder if something similar could be done for #91. Seems like some sort of abstraction for adding patches to the kube-apiserver config could also be useful.

@mkjpryor
Copy link
Collaborator

@heytrav I was thinking the same. Let me have a bit of a think about how it would work.

@mkjpryor mkjpryor mentioned this pull request Oct 2, 2023
Merged
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

@mkjpryor

Approval is required for workflow run #6412690116 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@heytrav
Copy link
Contributor Author

heytrav commented Oct 7, 2023 •
edited
Loading

@mkjpryor based the changes you made in #130 it looks like I can just override controlPlane.kubeadmConfigSpec and nodeGroupDefaults.kubeadmConfigSpec to add the necessary ignition components in a user defined -f values.yaml. I'm still experimenting with it to be sure.

For the purpose of this PR I'm wondering if you want to

  1. Provide any sort of conditional blocks in charts/openstack-cluster/templates/_helpers.tpl to switch between cloud-init or ignition based systems OR
  2. Just add some examples in the documentation and leave it up to the user to implement

This was referenced Oct 9, 2023
Open
Open
@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6490986359 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6499325307 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6513871433 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@heytrav
Copy link
Contributor Author

heytrav commented Oct 18, 2023

I've refactored the original PR to remove most of the changes I had made. In the current state it is possible to build Flatcar clusters with this code and providing customised values.yaml.

However as mentioned in #138 it was necessary to change the last command added to preKubeadmCommands because it references a folder that does not exist on Flatcar and causes the build to fail. Rather than remove it completely I added a separate definition that uses a different path when ignitionBasedOS: true.

To resolve #139 I changed the destination of the additional containerd config to create a new file under /etc/containerd/config.d/. Not sure if this is best approach but for now it at least doesn't clobber the containerd configuration

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6564698214 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@mkjpryor
Copy link
Collaborator

mkjpryor commented Oct 23, 2023 •
edited
Loading

@heytrav

I think in the short- to medium-term it would be nice to have the Flatcar support be native to the charts, so that we only require the user to flip a value in the charts between flatcar and ubuntu. However if you think this is the minimum set of changes that enable the use of Flatcar and you don't have the time or inclination to look at the required changes for that, I am happy to merge (subject to a full review and passing the test suite).

Let me know what you prefer.

@heytrav
Copy link
Contributor Author

heytrav commented Oct 23, 2023

@heytrav

I think in the short- to medium-term it would be nice to have the Flatcar support be native to the charts, so that we only require the user to flip a value in the charts between flatcar and ubuntu. However if you think this is the minimum set of changes that enable the use of Flatcar and you don't have the time or inclination to look at the required changes for that, I am happy to merge (subject to a full review and passing the test suite).

Let me know what you prefer.

I actually would like to have ignition based OS (i.e. flatcar) be completely handled by the chart as you suggest. However I wasn't sure how you preferred this to work.

I will need to experiment a bit to add this into the chart. If you have any suggestions, I'm happy to follow your lead.

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6621090579 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@heytrav
Copy link
Contributor Author

heytrav commented Oct 24, 2023

Most recent commit works for me locally (I can build a Flatcar cluster).

@mkjpryor please let me know if this is more in line with what you were thinking.

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6621111706 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

mkjpryor
mkjpryor requested changes Oct 24, 2023
View reviewed changes
charts/openstack-cluster/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/openstack-cluster/values.yaml Outdated Show resolved Hide resolved
@mkjpryor
Copy link
Collaborator

@heytrav

I added my thoughts in the form of review comments. Since we are adding support for a second OS, I would like to make sure that we can actually support N OSs going forward, just in case, so the suggestions I made reflect that.

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6634349692 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@heytrav
Copy link
Contributor Author

heytrav commented Oct 25, 2023

@heytrav

I added my thoughts in the form of review comments. Since we are adding support for a second OS, I would like to make sure that we can actually support N OSs going forward, just in case, so the suggestions I made reflect that.

pushed up some changes

@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6634731110 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

Modifications to get flatcar working
8861213 
* Move flatcar config spec into specialised define block
* generic osDistro variable to specify "flatcar" and future other OS
@github-actions
Copy link

@mkjpryor

Approval is required for workflow run #6636825855 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023 •
edited
Loading

@mkjpryor

Approval is required for workflow run #6636825855 for this PR.

Please review the code that will be executed by this workflow run and give either a 👍 or 👎 on this comment to approve or deny execution.

Workflow run approved by mkjpryor.

@mkjpryor mkjpryor self-requested a review October 26, 2023 11:19
mkjpryor
mkjpryor approved these changes Oct 26, 2023
View reviewed changes
Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkjpryor mkjpryor merged commit 13115d9 into azimuth-cloud:main Oct 26, 2023
13 checks passed
stefan-chivu referenced this pull request in stefan-chivu/magnum-capi-helm Nov 1, 2023
@stefan-chivu
changed values according to what is expected by https://github.com/st…
79c12d9 
…ackhpc/capi-helm-charts/pull/53
stefan-chivu referenced this pull request in stefan-chivu/magnum-capi-helm Nov 1, 2023
@stefan-chivu
changed values according to what is expected by https://github.com/st…
c27974e 
…ackhpc/capi-helm-charts/pull/53
dalees referenced this pull request in dalees/magnum-capi-helm Nov 2, 2023
@dalees
Add support for Flatcar OS
b6534f1 
Fetch os_distro metadata and pass to helm chart.

Depends on Helm chart flatcar support:
https://github.com/stackhpc/capi-helm-charts/pull/53
dalees referenced this pull request in dalees/magnum-capi-helm Nov 2, 2023
@dalees
Add support for Flatcar OS
8634f00 
Fetch os_distro metadata and pass to helm chart.

Depends on Helm chart flatcar support:
https://github.com/stackhpc/capi-helm-charts/pull/53
@dalees dalees mentioned this pull request Nov 2, 2023
Merged
dalees referenced this pull request in dalees/magnum-capi-helm Nov 17, 2023
@dalees
Add support for Flatcar OS
73b44d0 
Fetch os_distro metadata and pass to helm chart.

Depends on Helm chart flatcar support:
https://github.com/stackhpc/capi-helm-charts/pull/53
@heytrav heytrav deleted the flatcar-integration branch November 22, 2023 02:33
JohnGarbutt referenced this pull request in stackhpc/magnum-capi-helm Nov 22, 2023
@dalees
Add support for Flatcar OS (#22)
b0765ea 
Fetch os_distro metadata and pass to helm chart.

Depends on Helm chart flatcar support:
https://github.com/stackhpc/capi-helm-charts/pull/53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkjpryor mkjpryor mkjpryor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@heytrav @mkjpryor