[WIP] ✨ Add support for Ignition v3.x #9158

bengentil · 2023-08-09T17:29:16Z

What this PR does / why we need it:

Implements Ignition v3.x

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #9157

k8s-ci-robot · 2023-08-09T17:29:25Z

Hi @bengentil. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-08-09T17:29:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign killianmuldoon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bengentil · 2023-08-09T17:59:16Z

Hi @invidian,

To follow the slack conversation we had, I pushed this PR so we can discuss the implementation in details.

The PR is working but I have a few questions:

I've added a new optional enum Transpiler and a ButaneConfig (similar to ContainerLinuxConfig), is this how you pictured the switch between clc & butane?
The butane implementation is very similar to the clc implementation, I think some templating functions can be shared (defaultTemplateFuncMap, mountpointName, templateYAMLIndent, parseOwner) in maybe a third package, for the rest I think it's way simpler and readable to keep 2 separate implementation, what do you think?

invidian

Hey @bengentil, thanks for opening this PR, this is highly appreciated! To answer your questions:

I've added a new optional enum Transpiler and a ButaneConfig (similar to ContainerLinuxConfig), is this how you pictured the switch between clc & butane?

Not really, I explained that in more details in a comment.

The butane implementation is very similar to the clc implementation, I think some templating functions can be shared (defaultTemplateFuncMap, mountpointName, templateYAMLIndent, parseOwner) in maybe a third package, for the rest I think it's way simpler and readable to keep 2 separate implementation, what do you think?

There is naturally a lot in common indeed, so definitely putting common parts in one place would be great, although I do not treat it as a must have.

However, looking at

cluster-api/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go

Line 210 in aac6da7

    
           func (c *KubeadmConfigSpec) validateIgnition(pathPrefix *field.Path) field.ErrorList {

, I'm wondering if perhaps Butane does not allow us to translate more features of cloudconfig into Ignition, which would introduce some differences between Butane and CLC. This is perhaps something we could look at.

Overall PR looks in a good shape to me, it's great to see it finally happening. Once it lands, perhaps Flatcar templates can be updated to use it so we can gradually phase out CLC and also Infra providers will be able to add support for FCOS.

invidian · 2023-08-10T07:40:48Z

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

+	// Ignition version, Default to clc
+	// +kubebuilder:validation:Enum=clc;butane
+	// +optional
+	Transpiler string `json:"transpiler,omitempty"`


Do we really need this field? Can't we just validate that only one of ContainerLinuxConfig and ButaneConfig is not nil at a time?

I think this is required for OS who doesn't support both implementation (v2.x only like Flatcar LTS, v3.x only like FCOS)

Relying on ContainerLinuxConfig and ButaneConfig being not nil is not sufficient as both can be nil (eg. I want to install FCOS with no additional config)

We can also make one of them required in Ignition type then, so user have to choose? We might also want to document that Butane supports generating Ignition v3 while CLC only v2.

IMO it boils down to: clc: {}/butane: {} vs transpiler: clc/transpiler: bunane, but for me the latter makes API more complex as there are 2 knobs for controlling one thing and it creates a matrix of additional bad configurations e.g. transpiler: clc with butane: {} in addition to existing scenarios like butane: {} and clc: {}, which we cannot avoid (they are essential).

I understand you point but what I propose is basically the same as we have in KubeadmConfigSpec:

a field Format to switch between "cloud-init" and "ignition"

a field Ignition used to pass optional ignition configs only when Format is explicitly set to "ignition"

Knowing that the Ignition field is optional, making one of clcConfig/butaneConfig required would means any existing declaration without additional config would have to add:

ignition: clcConfig: {}

(or clc if the field is renamed)
And when ignition v2.x is phased out from flatcar, a new change would be required to:

ignition: butaneConfig: {}

I'm not sure this is a good UX, what I proposed initially was an enum version field to be self-explanatory about the Ignition version the user want (and let the implementation decide the correct transpiler to call), I changed it to a transpiler field to be consistent with the clcConfig and butaneConfig fields but the long-term logic is the same:

first we introduce a new optional transpiler, so any user who want to start using Ignition v3 can (no impact on existing manifests)

once Flatcar LTS updates to Ignition v3, butane becomes the default transpiler (when declared, additionalConfig must be moved from clcConfig to butaneConfig in a variant/version supported by butane or the transpiler field should be explicitly set to clc)

at some point, clc implementation is phased out

That's a good point, I didn't think about the backward compatibility initially. So how about still picking CLC as a default if both clcConfig and butaneConfig are nil? This remains backward compatible and users which needs Ignition v3 can specify butaneConfig: {}.

It does however make selection of Ignition version kind of ambiguous, which I'm not a fan of. Having an enum for Ignition version with v2 default for the time being and v3 as a new option as you originally suggested does make sense in this case. So for users without additional configuration, we would use CLC as a default transpiler for v2 and Butane for v3, but that would be an implementation detail. I also wonder if it make sense to support full versions like 2.3.0 or we could get away with just v2/v3 distinction for the time being.

At the same time, requesting Ignition v3 with CLC configuration would result in an error, as CLC is not capable of producing it. The similar validation would need to be added for Butane.

It would be good for someone else to tip in, maybe @pothos or @jepio could have a look with their critical eye :) And definitely someone from CAPI maintainers.

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

invidian · 2023-08-10T07:43:34Z

bootstrap/kubeadm/internal/ignition/butane/butane.go

+
+const (
+	butaneTemplate = `---
+variant: flatcar


I don't think this should be hardcoded here. Perhaps it should be an API field?

This is the variant & version of the static template only, the additionalConfig must specify it's own variant/version, both are translated to ignition v3.4.0 and the merge is done in ignition v3.4.0.

That seem counter-intuitive, as additionalConfig name suggests it's optional and so is the API semantics for this field. How do we require it right now? Shouldn't we require this information from user and use it then for both base template and additional config?

additionalConfig is still optional, the variant/version is required by butane when additionalConfig is not nil to transpile it to Ignition v3.4.0.

This version doesn't have to be the same as the one used in the static template as the merge is done in Ignition v3.4.0, so a user can add an additionalConfig in any variant/version they want (as long as the couple can be transpiled to Ignition v3.4.0)

We could add a default variant/version if none is specified as it would work in most cases.

I understand that, but if user specifies no additionalConfig, why should they be forced to use flatcar variant in this case? What if this variant is not compatible with their OS?

invidian · 2023-08-10T07:43:43Z

bootstrap/kubeadm/internal/ignition/butane/butane.go

+const (
+	butaneTemplate = `---
+variant: flatcar
+version: 1.1.0


Should this be an API field as well?

Same as above

bootstrap/kubeadm/internal/ignition/butane/butane.go

invidian · 2023-08-10T08:00:24Z

bootstrap/kubeadm/internal/ignition/ignition.go

+	if ignitionConfig != nil && ignitionConfig.Transpiler == "butane" {
+		butaneConfig := &bootstrapv1.ButaneConfig{}
+		if ignitionConfig.ButaneConfig != nil {
+			butaneConfig = ignitionConfig.ButaneConfig
+		}
+		return butane.Render(input, butaneConfig, kubeadmConfig)
+	}
 	return clc.Render(input, clcConfig, kubeadmConfig)


What if user specifies both ignitionConfig and butaneConfig at the same time? IMO it's a bad configuration, as it contains no-op parts and should return an error to the user.

I think we should add additional validation to

cluster-api/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go

Line 210 in aac6da7

func (c *KubeadmConfigSpec) validateIgnition(pathPrefix *field.Path) field.ErrorList {

to only accept one configuration format at a time and here simply do something like:

Suggested change

if ignitionConfig != nil && ignitionConfig.Transpiler == "butane" {

butaneConfig := &bootstrapv1.ButaneConfig{}

if ignitionConfig.ButaneConfig != nil {

butaneConfig = ignitionConfig.ButaneConfig

}

return butane.Render(input, butaneConfig, kubeadmConfig)

}

return clc.Render(input, clcConfig, kubeadmConfig)

if ignitionConfig.ButaneConfig != nil {

return butane.Render(input, ignitionConfig.ButaneConfig, kubeadmConfig)

}

return clc.Render(input, clcConfig, kubeadmConfig)

invidian · 2023-08-10T08:08:53Z

/ok-to-test

implements kubernetes-sigs#9157

fabriziopandini · 2023-08-13T10:47:44Z

If I remember well when we implemented ingition we agreed that this was a temporary solution, while a more long-term solution for machine/node bootstrap should be worked out in order to avoid CABPK becoming unmaintainable, its API exploding, the main CAPI repo to pull too many dependency.

I think we should revive this discussion before moving forward with this PR, before adding complexity on top of a situation that we know already not ideal

cc @vincepri @CecileRobertMichon @enxebre @sbueringer @killianmuldoon @invidian @dongsupark @johananl

johananl · 2023-08-14T11:15:45Z

Yes @fabriziopandini, I think this is the relevant issue: #5294

k8s-ci-robot · 2023-08-15T17:46:47Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-triage-robot · 2024-01-21T02:11:54Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

johananl · 2024-01-22T10:13:51Z

IMO we can freeze/close this PR for now. #5294 is WIP and AFAICT we can't advance this PR until that design proposal is settled.

vincepri · 2024-01-22T16:40:39Z

@fabriziopandini @johananl @invidian @bengentil Ignition v2 has long standing issues as today, given that these APIs are still behind a feature gate, I can see why adding support for v3 going forward would be beneficial.

That said, I'd personally like to see a more cohesive approach to Ignition support within Cluster API, on the Machine object specifically.

johananl · 2024-01-22T17:21:19Z

That said, I'd personally like to see a more cohesive approach to Ignition support within Cluster API, on the Machine object specifically.

Yes, this is one of the core areas we're trying to address in #5294. I'm definitely taking that into account in my WIP design proposal. I'll share a draft as soon as I can.

k8s-ci-robot · 2024-01-26T17:36:48Z

@bengentil: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-verify-main	`7ffba84`	link	true	`/test pull-cluster-api-verify-main`
pull-cluster-api-build-main	`7ffba84`	link	true	`/test pull-cluster-api-build-main`
pull-cluster-api-e2e-informing-main	`7ffba84`	link	false	`/test pull-cluster-api-e2e-informing-main`
pull-cluster-api-e2e-main	`7ffba84`	link	true	`/test pull-cluster-api-e2e-main`
pull-cluster-api-test-main	`7ffba84`	link	true	`/test pull-cluster-api-test-main`
pull-cluster-api-e2e-blocking-main	`7ffba84`	link	true	`/test pull-cluster-api-e2e-blocking-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-triage-robot · 2024-02-25T18:35:26Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-03-26T19:15:53Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-03-26T19:15:58Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bengentil · 2024-03-28T13:50:15Z

/reopen

k8s-ci-robot · 2024-03-28T13:50:19Z

@bengentil: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2024-03-28T13:50:25Z

This PR is currently missing an area label, which is used to identify the modified component when generating release notes.

Area labels can be added by org members by writing /area ${COMPONENT} in a comment

Please see the labels list for possible areas.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-triage-robot · 2024-04-27T14:40:46Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-04-27T14:40:50Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bengentil · 2024-04-29T10:33:49Z

/reopen

k8s-ci-robot · 2024-04-29T10:33:53Z

@bengentil: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sbueringer · 2024-04-29T10:45:16Z

What is the current status of this PR? I'm not sure what the next steps are to make progress with this PR/topic.

fabriziopandini · 2024-04-29T12:13:00Z

I'm personally -1 to keep this PR open.
The last commit is from Aug 2023; most important, by looking at the comments above it also seems there are open discussions to be addressed (a potentially a new proposal to be discussed according to #9158 (comment)) before making further progress

bengentil · 2024-04-29T15:41:07Z

I was waiting for progress on #5294 as there were concerns about the maintainability of the current CABPK implementation.

If we don't want to wait for the rework described in #5294 to gain ignition v3 (and FCOS) support, I can rebase and continue to work on this, otherwise we can close this PR.

fabriziopandini · 2024-04-29T19:17:52Z

Based on the latest discussion in the community I'm aware of about this topic, we should wait for progress on #5294 first, but feel free to bring it up again in the office hours given that some time is passed now and folks might have different opinions

/close

k8s-ci-robot · 2024-04-29T19:17:56Z

@fabriziopandini: Closed this PR.

In response to this:

Based on the latest discussion in the community I'm aware of about this topic, we should wait for progress on #5294 first, but feel free to bring it up again in the office hours given that some time is passed now and folks might have different opinions

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

johananl · 2024-05-02T05:27:54Z

Just for context, #5294 touches some very basic assumptions in the CAPI design and it will therefore likely take some time to agree on the design and finish the implementation.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 9, 2023

k8s-ci-robot requested review from enxebre and jackfrancis August 9, 2023 17:29

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2023

bengentil force-pushed the ben/ignitionv3 branch from f90fa39 to aac6da7 Compare August 9, 2023 17:44

invidian suggested changes Aug 10, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2023

bengentil force-pushed the ben/ignitionv3 branch from aac6da7 to 5642eb9 Compare August 10, 2023 09:41

bootstrap/kubeadm: Add support for Ignition v3.x

7ffba84

implements kubernetes-sigs#9157

bengentil force-pushed the ben/ignitionv3 branch from 5642eb9 to 7ffba84 Compare August 10, 2023 09:51

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2023

johananl mentioned this pull request Aug 22, 2023

Add support for Ignition v3 flatcar/Flatcar#942

Open

bengentil mentioned this pull request Sep 4, 2023

Ignition v3.x support #9157

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 25, 2024

k8s-ci-robot closed this Mar 26, 2024

k8s-ci-robot reopened this Mar 28, 2024

k8s-ci-robot added the do-not-merge/needs-area PR is missing an area label label Mar 28, 2024

k8s-ci-robot closed this Apr 27, 2024

k8s-ci-robot reopened this Apr 29, 2024

k8s-ci-robot closed this Apr 29, 2024

defo89 mentioned this pull request Aug 12, 2024

Convert Ignition v2.x to v3.x to support CAPBK ironcore-dev/boot-operator#88

Open

[WIP] ✨ Add support for Ignition v3.x #9158

[WIP] ✨ Add support for Ignition v3.x #9158

Conversation

bengentil commented Aug 9, 2023

k8s-ci-robot commented Aug 9, 2023

k8s-ci-robot commented Aug 9, 2023

bengentil commented Aug 9, 2023

invidian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengentil Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian commented Aug 10, 2023

fabriziopandini commented Aug 13, 2023

johananl commented Aug 14, 2023

k8s-ci-robot commented Aug 15, 2023

k8s-triage-robot commented Jan 21, 2024

johananl commented Jan 22, 2024

vincepri commented Jan 22, 2024

johananl commented Jan 22, 2024

k8s-ci-robot commented Jan 26, 2024

k8s-triage-robot commented Feb 25, 2024

k8s-triage-robot commented Mar 26, 2024

k8s-ci-robot commented Mar 26, 2024

bengentil commented Mar 28, 2024

k8s-ci-robot commented Mar 28, 2024

k8s-ci-robot commented Mar 28, 2024

k8s-triage-robot commented Apr 27, 2024

k8s-ci-robot commented Apr 27, 2024

bengentil commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

sbueringer commented Apr 29, 2024

fabriziopandini commented Apr 29, 2024

bengentil commented Apr 29, 2024

fabriziopandini commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

johananl commented May 2, 2024

bengentil Aug 10, 2023 •

edited

Loading