Create machine_config_to_ignition #2960

mkenigs · 2022-02-22T15:01:21Z

Closes https://issues.redhat.com/browse/MCO-184

mkenigs · 2022-02-22T15:04:59Z

pkg/controller/render/machine_config_to_ignition.go

+type OriginFile struct {
+	Packages       []string `yaml:"packages"`
+	OverrideRemove []string `yaml:"override-remove"`
+}
+
+func (originFile OriginFile) isEmpty() bool {
+	return len(originFile.Packages) == 0 && len(originFile.OverrideRemove) == 0
+}
+
+func (originFile OriginFile) toIgnFile(allowCompression bool) (ign3types.File, error) {
+	treeFileContents, err := yaml.Marshal(originFile)
+	if err != nil {
+		return ign3types.File{}, fmt.Errorf("failed to marshal extensions as yaml: %w", err)
+	}
+	fullYamlContents := append([]byte("# Generated by the MCO\n\n"), treeFileContents...)
+	src, gzipped, err := baseutil.MakeDataURL(fullYamlContents, nil, allowCompression)
+	if err != nil {
+		return ign3types.File{}, fmt.Errorf("could not encode extensions file: %w", err)
+	}
+	hash := sha256.New()
+	hash.Write([]byte(src))
+	sha := hex.EncodeToString(hash.Sum(nil))[0:7]
+	file := ign3types.File{
+		Node: ign3types.Node{
+			Path: "/etc/rpm-ostree/origin.d/extensions-" + sha + ".yaml",
+		},
+		FileEmbedded1: ign3types.FileEmbedded1{
+			Contents: ign3types.Resource{
+				Source: util.StrToPtr(src),
+			},
+			Mode: util.IntToPtr(0644),
+		},
+	}
+	if gzipped {
+		file.Contents.Compression = util.StrToPtr("gzip")
+	}
+
+	return file, nil
+}


@jmarrero @jlebon this is mostly just copy pasted from @jmarrero 's code, but I did tweak it to allow OverrideRemove.

Is there somewhere we could move this to that both the MCO and Butane could use? Seems like "create an Ignition file with the first 8 characters of the hash of its contents added to its path" is not a very clean API. Maybe I just need to get over that for now 😂

Is this correct syntax for override-remove?

I'm using github.com/coreos/butane/base/util and github.com/coreos/ignition/v2/config/util, is that okay? On a related note, I wonder if some Ignition helpers should be moved to a lib? Eg the MCO has helpers like https://github.com/openshift/machine-config-operator/blob/master/test/helpers/helpers.go#L155. That helper uses EncodeBytes from github.com/vincent-petithory/dataurl while the Butane code is using baseutil.MakeDataURL. Which is leading to different encodings being used, which I ran into when writing the unit tests

The reason I used a SHA with the first characters is trying to mimic how github shows commits. The SHA is not really important the important part is that the filename is unique and reproducible. If the MCO is doing something similar somewhere else or have a better approach it's ok to change it. rpm-ostree does not care about the filename. I am not sure if there is a good place to put shared code or if this is big enough to put in a library of sorts.

Looks like it: https://github.com/coreos/rpm-ostree/blob/3c64d05bc5b98e225f2823dbfec3a2adb1bb4411/rust/src/treefile.rs#L1832

Re. 2, override-remove is not supported yet in the CoreOS layering flow, but I'm working on it! :)

related coreos/rpm-ostree#3364

Also coreos/rpm-ostree#2542

Having a chat with @mkenigs about this, one idea here is to update the MCO's vendor of fcct ➡️ butane, and then generate butane for this fragment. @mkenigs is suggesting alternatively that butane exposes a Go API to generate the origin file directly.

Chatted with @mkenigs about this too. Generating a Butane fragment and then rendering that seems fine, but personally my vote is to keep those ~30 duplicated lines here for now as we iterate on getting something working and add a TODO we can circle back to. Once we gain more confidence in how everything is connected, we can start looking at optimizing and deduping things.

Just to xref, #2976 is going to update our butane vendoring, which would unblock the choice to either generate Butane directly, or expose a Go API in butane.

mkenigs · 2022-02-22T15:12:22Z

/assign @jkyros @yuqi-zhang

mkenigs · 2022-02-22T15:16:57Z

pkg/apis/machineconfiguration.openshift.io/v1/types.go


+// MCDContent
+type MCDContent struct {


Is factoring this out worth the churn? Also what do we want to call this? There's a chance we may even drop it depending on the outcome of https://issues.redhat.com/browse/MCO-193

I think I'd vote not to do the churn on this now.

Dropped that commit but did move that struct to machine_config_to_ignition.go

mkenigs · 2022-03-04T00:51:04Z

Not sure if we want to wait for #2976, but I think this should be ready to go into layering

mkenigs · 2022-03-07T22:21:27Z

Commits from #2976 fix the unit test this is currently breaking due to Ignition version bump

mkenigs · 2022-03-17T19:43:24Z

Should be good to go now that #2976 is in

cgwalters

Let's consider unifying this with the encapsulated machineconfig.

cgwalters · 2022-03-22T22:06:15Z

pkg/controller/render/machine_config_to_ignition.go

+	"gopkg.in/yaml.v3"
+)
+
+const MCDContentPath = "/etc/machine-config-daemon/mcd_content.json"


This one definitely deserves a comment. It seems strongly related to
MachineConfigEncapsulatedPath = "/etc/ignition-machine-config-encapsulated.json"

In fact, any reason not to make them the same?

I'm not familiar with how that path is used but it appears to be used to signal completion of firstboot: https://github.com/openshift/machine-config-operator/blob/master/pkg/daemon/daemon.go#L614

Could also potentially deduplicate some code with appendEncapsulated

I was keeping everything separate so we don't break anything but can combine if that makes more sense

See #868 - the commit message there tries to explain this

Anyways though...I think you're right that because this file is involved in special "firstboot" semantics, perhaps we should use a separate one to be safer.

pkg/controller/render/machine_config_to_ignition.go

pkg/controller/render/machine_config_to_ignition_test.go

cgwalters · 2022-03-29T19:34:24Z

pkg/controller/render/machine_config_to_ignition.go

+const MCDContentPath = "/etc/machine-config-daemon/mcd_content.json"
+
+type MCDContent struct {
+	KernelArguments []string `json:"kernelArguments"`


Worth noting that this exists in Ignition 3.3, and then if we agreed on coreos/ignition#1323 ...the need for this would entirely go away.

So...hmm, I am not sure where we are on an update to Ignition spec 3.3. It may actually block on newer bootimages 😢

Is that worth creating a card or something?

cgwalters · 2022-03-29T19:36:02Z

pkg/controller/render/machine_config_to_ignition.go

+	}
+
+	// KernelType
+	if ctrlcommon.CanonicalizeKernelType(mcSpec.KernelType) == ctrlcommon.KernelTypeRealtime {


Feels like this one should be a switch or so and we explicitly no-op on the default kernel type? But not a blocker.

Fixed, should I return err for default?

pkg/controller/render/machine_config_to_ignition_test.go

cgwalters · 2022-03-29T19:41:40Z

/approve
/lgtm

mkenigs · 2022-03-29T20:29:19Z

/hold

mkenigs · 2022-03-29T20:47:21Z

/hold cancel

mkenigs · 2022-03-31T19:55:57Z

@cgwalters made changes according to your suggestions, can you re-approve?

mkenigs · 2022-03-31T19:56:32Z

Also left some responses to your comments above

jkyros

/lgtm

openshift-ci · 2022-03-31T23:49:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jkyros, mkenigs

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters]

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

openshift-ci · 2022-04-01T02:06:32Z

@mkenigs: 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
ci/prow/e2e-gcp-op-single-node	`006d3a9`	link	false	`/test e2e-gcp-op-single-node`
ci/prow/e2e-aws-upgrade	`006d3a9`	link	false	`/test e2e-aws-upgrade`
ci/prow/e2e-gcp-single-node	`006d3a9`	link	false	`/test e2e-gcp-single-node`

Full PR test history. Your PR dashboard.

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.

openshift-bot · 2022-04-01T03:02:04Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

mkenigs marked this pull request as draft February 22, 2022 15:01

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 22, 2022

openshift-ci bot requested review from cgwalters and sinnykumari February 22, 2022 15:04

mkenigs commented Feb 22, 2022

View reviewed changes

openshift-ci bot assigned jkyros and yuqi-zhang Feb 22, 2022

mkenigs commented Feb 22, 2022

View reviewed changes

mkenigs force-pushed the translate-184 branch from 3cda441 to 3eeb95d Compare February 22, 2022 15:27

mkenigs force-pushed the translate-184 branch 2 times, most recently from c68ecfc to 513653e Compare March 4, 2022 00:44

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2022

mkenigs changed the base branch from mcbs to layering March 4, 2022 00:45

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2022

mkenigs force-pushed the translate-184 branch from 513653e to 08b4040 Compare March 4, 2022 00:49

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2022

mkenigs marked this pull request as ready for review March 4, 2022 00:50

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2022

openshift-ci bot requested review from kikisdeliveryservice and yuqi-zhang March 4, 2022 00:50

cgwalters added the layering label Mar 7, 2022

mkenigs marked this pull request as draft March 7, 2022 22:20

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2022

mkenigs force-pushed the translate-184 branch from 08b4040 to 034ed01 Compare March 7, 2022 22:20

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2022

mkenigs force-pushed the translate-184 branch from 034ed01 to 559a069 Compare March 16, 2022 21:33

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2022

openshift-ci bot requested a review from jkyros March 16, 2022 21:35

mkenigs added 2 commits March 22, 2022 11:54

Move getSupportedExtensions to ctrlcommon

626d066

Move CanonicalizeKernelType to ctrlcommon

4f29bf4

mkenigs force-pushed the translate-184 branch from fe7a330 to 6e9e138 Compare March 22, 2022 16:55

cgwalters reviewed Mar 22, 2022

View reviewed changes

mkenigs force-pushed the translate-184 branch from 6e9e138 to b4c0195 Compare March 28, 2022 20:21

cgwalters reviewed Mar 29, 2022

View reviewed changes

cgwalters approved these changes Mar 29, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022

openshift-ci bot assigned cgwalters Mar 29, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2022

Create machine_config_to_ignition{_test}.go

006d3a9

mkenigs force-pushed the translate-184 branch from b4c0195 to 006d3a9 Compare March 29, 2022 20:46

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2022

jkyros approved these changes Mar 31, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2022

openshift-merge-robot merged commit 10957fc into openshift:layering Apr 1, 2022

mkenigs deleted the translate-184 branch April 1, 2022 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create machine_config_to_ignition #2960

Create machine_config_to_ignition #2960

mkenigs commented Feb 22, 2022

mkenigs Feb 22, 2022 •

edited

Loading

mkenigs Feb 22, 2022

jmarrero Feb 22, 2022

jmarrero Feb 22, 2022

jlebon Feb 28, 2022

cgwalters Feb 28, 2022

cgwalters Feb 28, 2022

cgwalters Feb 28, 2022

jlebon Mar 1, 2022

cgwalters Mar 3, 2022

mkenigs commented Feb 22, 2022

mkenigs Feb 22, 2022

cgwalters Mar 3, 2022

mkenigs Mar 4, 2022

mkenigs commented Mar 4, 2022

mkenigs commented Mar 7, 2022

mkenigs commented Mar 17, 2022

cgwalters left a comment

cgwalters Mar 22, 2022

mkenigs Mar 28, 2022

cgwalters Mar 29, 2022

cgwalters Mar 29, 2022

mkenigs Mar 29, 2022

cgwalters Mar 29, 2022

mkenigs Mar 29, 2022

cgwalters commented Mar 29, 2022

mkenigs commented Mar 29, 2022

mkenigs commented Mar 29, 2022

mkenigs commented Mar 31, 2022

mkenigs commented Mar 31, 2022

jkyros left a comment

openshift-ci bot commented Mar 31, 2022

openshift-ci bot commented Apr 1, 2022 •

edited

Loading

openshift-bot commented Apr 1, 2022

Create machine_config_to_ignition #2960

Create machine_config_to_ignition #2960

Conversation

mkenigs commented Feb 22, 2022

mkenigs Feb 22, 2022 • 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

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

mkenigs commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkenigs commented Mar 4, 2022

mkenigs commented Mar 7, 2022

mkenigs commented Mar 17, 2022

cgwalters 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

cgwalters commented Mar 29, 2022

mkenigs commented Mar 29, 2022

mkenigs commented Mar 29, 2022

mkenigs commented Mar 31, 2022

mkenigs commented Mar 31, 2022

jkyros left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 31, 2022

openshift-ci bot commented Apr 1, 2022 • edited Loading

openshift-bot commented Apr 1, 2022

mkenigs Feb 22, 2022 •

edited

Loading

openshift-ci bot commented Apr 1, 2022 •

edited

Loading