Integrate functionality from update into layered update by mkenigs · Pull Request #3030 · openshift/machine-config-operator

mkenigs · 2022-03-21T17:45:04Z

Still rebasing changes to experimentalUpdateLayeredConfig that will use these changes. Should I lump it all into one PR or keep this as a separate prep PR?

mkenigs · 2022-03-21T17:47:25Z

This feels like overkill and I'm not even sure ostree's current behavior is intended, but right now ostree prints out every directory for a single file change, eg:

/path /path/to /path/to/file

Not sure if there's an easier way to simplify that than a tree

The reason this happens is because ostree is content-addressed; each directory is represented by a checksum. So any change to any file recursively changes every directory object up to the parent.

(Just like git)

So that's why we print out each directory as modified.

Gotcha makes sense. Do you think generating a tree is a good approach or too complicated? I think @jkyros is okay with it but would prefer something easier to understand

cgwalters · 2022-03-21T19:28:44Z

/approve

cgwalters · 2022-03-21T19:29:18Z

 	return
 }

+func Cat(stateroot, commit, path string) ([]byte, error) {


We could add a Deployment struct and this as a method; e.g. generate those structs from the status output.

If you did booted.Cat(path), would it be clear whether that would cat what was originally booted vs live applied?

Oh good question! Offhand I'd say booted.Cat(path) should always operate on the booted commit. Querying the live filesystem state inherently doesn't need a special API, one can just call open(path) right?

True. I'm trying to diff two files from two different ostree commits though, but it's possible that one of them is live applied. Because if that I think I don't think it really makes sense to be on a Deployment struct because that doesn't really make sense when I need to cat a live applied file. I could just open(path) 🤔 But since I'm calling ostree diff image1 image2 it feels like diff(cat(image1, path), cat(image2, path)) is a bit more consistent. And right now I implemented Cat as just calling open

mkenigs · 2022-03-21T22:57:41Z

Okay I'm going to make this a mega PR with everything I've been working on for now because some of my changes won't even build without others. This is rebased on top of a few other PRs so it should gradually get smaller

jkyros

So it looks like this kinda:

fixes up the build configs to squash layers and use machine_config_to_ignition
adjusts drain functions to use the new "file read" functions that return lists of files instead of parsing the ignition files directly (this shouldn't break anything, but it does adjust an existing code path)
updates the "rpm-ostree bindings" to process the file diff
fixes my experimentalUpdateLayeredConfig() sloppy mess to work properly with drain/liveapply :)

jkyros · 2022-03-29T18:53:07Z

I don't care that the comment got mangled, but I am just mentioning it in case it is a find/replace or copy/paste accident. :)

Haha I have no idea how that happened. Fixed

jkyros · 2022-03-29T20:01:29Z

It might just be the way my brain works, but this totally baked my noodle. I looked at it for way too long. 😆

Am I understanding this right that the reason you do all this work parsing the diff into a FileTree is because:

ostree diff includes the intermediate directories also

you don't know which ones are the intermediate directories until you find the "leaves" ?

once you find the leaves, you can figure out which ones are "extra" (non-leaf) and exclude them from the changeset?

It might just be the way my brain works, but this totally baked my noodle. I looked at it for way too long. laughing

Yeah see my comment above about overkill 😬

Am I understanding this right that the reason you do all this work parsing the diff into a FileTree is because:

ostree diff includes the intermediate directories also

Yep

you don't know which ones are the intermediate directories until you find the "leaves" ?

Right. We know a directory is not intermediate if it has no children or equivalently len(pathsToLeaves) == 0

once you find the leaves, you can figure out which ones are "extra" (non-leaf) and exclude them from the changeset?

Essentially. The recursion will result in going from children up to the root, so you don't exclude so much as prepend to an already existing path. E.g. if we have /path/to/file, when you're processing to you'll see that you have child file and put /to/file in paths. And then the same will happen to prepend path

Any ideas on making this more legible? Or taking a completely different approach? I'm not even sure if ostree outputting every directory is a bug or a feature. @cgwalters should ostree be outputting

/path /path/to /path/to/file

jkyros · 2022-03-29T20:10:01Z

It's less gross, and it's not an array anymore, but it's still here! 😄

(This is just commentary, I assume we'll leave this here until we decide whether to break some of this out into some sort of library/bindings or not)

Haha yep just needed to have Deployment vars

jkyros · 2022-03-29T20:32:34Z

When I hacked on this I was trying to decide if I should find a way to mangle SetDone to satisfy both paths, but I am very in favor of this being separate for now.

We're eventually going to have to talk about the pool/node-controller consequences of how to get from where we are now with layering which is:

Our pool gets to a MachineConfig via an image

To the desired and state where:

Our pool gets to an Image (that just so happens to contain the required machineconfig)

Like eventually the image needs to be the config, not the rendered-config.

Right now pool.Spec.Configuration.Name is implied to be a MachineConfig. As part of the build controller-y stuff I was thinking through what that might mean if the pool's config is an actual image.

Do we just add a "Kind" there in the pool status/spec to go along with the name? And then the pool/node-controller is like "I got you to that config you specified, whatever kind it was". Definitely a longer discussion that crosses into UX.

This comment is too long, but it's too late.

Hmm yeah I'm less familiar with the controller side, I think I'd like to talk through some of that. From the MCD side, I think it would be nice if we could drop the SetDone call, because then all the MCD has to care about is 1. What's my DesiredImageConfigAnnotationKey and 2. did I set MachineConfigDaemonStateAnnotationKey and CurrentImageConfigAnnotationKey

Add a comment to Rebase Don't return changed since there should be an error if nothing changed Remove logs about pivot and custom origin since we don't use pivot:// or --custom-origin-url for layered rebases

Also add LiveReplaced field A layered update will need to perform operations on the staged and deployed Deployments, which will require variables of type Deployment

mkenigs · 2022-04-04T16:28:00Z

/test images

mkenigs · 2022-04-04T17:05:10Z

/retest-required

Factored some code out of experimentalUpdateLayeredConfig into a function in daemon.go since eventually we'll want to write pull secret on daemon startup. We don't handle layering there currently so for now we'll continue to call the function in experimentalUpdateLayeredConfig Also moved getPullSecret to daemon

Translate MachineConfig to Ignition and pass to mco-build-content build

Currently the Docker build leaves behind some files that trigger a reboot when we should be applying live, so those files should be cleaned up instead. Set ImageOptimizationPolicy=ImageOptimizationSkipLayers since currently ostree does not support whiteouts so file deletions won't work unless layers are squashed

openshift-ci · 2022-04-04T23:55:47Z

@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	`8d73a1f`	link	false	`/test e2e-gcp-op-single-node`
ci/prow/e2e-gcp-single-node	`8d73a1f`	link	false	`/test e2e-gcp-single-node`

Full PR test history. Your PR dashboard.

Details

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.

jkyros · 2022-04-05T16:30:26Z

The "tree stuff" for the diff parsing works for now, worst case we put a big fat explanation comment on it if we keep it. I'd rather not hold things up just for that.

/lgtm

openshift-ci · 2022-04-05T16:31:04Z

[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

Details

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

mkenigs commented Mar 21, 2022

View reviewed changes

openshift-ci Bot requested review from kikisdeliveryservice and sinnykumari March 21, 2022 17:47

cgwalters reviewed Mar 21, 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 21, 2022

cgwalters reviewed Mar 21, 2022

View reviewed changes

Comment thread pkg/daemon/rpm-ostree_test.go Outdated

mkenigs force-pushed the layering-rpm-ostree branch 3 times, most recently from cef9b33 to b957ffd Compare March 21, 2022 22:56

mkenigs changed the title ~~rpm-ostree prep work for layering branch~~ Integrate functionality from update into layered update Mar 21, 2022

mkenigs marked this pull request as draft March 21, 2022 22:57

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 21, 2022

mkenigs force-pushed the layering-rpm-ostree branch from b957ffd to 4556f31 Compare March 22, 2022 14:18

mkenigs commented Mar 22, 2022

View reviewed changes

Comment thread pkg/daemon/rpm-ostree.go Outdated

LorbusChris mentioned this pull request Mar 25, 2022

RFC: Add OKD machine-os definition using CoreOS Layering coreos/fedora-coreos-config#1622

Closed

mkenigs force-pushed the layering-rpm-ostree branch 3 times, most recently from 8551b64 to 9e0ea00 Compare March 29, 2022 18:38

jkyros reviewed Mar 29, 2022

View reviewed changes

Comment thread pkg/daemon/update.go Outdated

mkenigs force-pushed the layering-rpm-ostree branch 5 times, most recently from 5bdc530 to 15ce427 Compare March 31, 2022 22:42

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

rpm-ostree.go: add Cat()

4592a9f

mkenigs force-pushed the layering-rpm-ostree branch from 15ce427 to 1ce5317 Compare April 1, 2022 20:27

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

mkenigs force-pushed the layering-rpm-ostree branch 2 times, most recently from c5a7f44 to 6f56f80 Compare April 1, 2022 20:48

mkenigs marked this pull request as ready for review April 1, 2022 20:52

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 Apr 1, 2022

mkenigs added 4 commits April 1, 2022 16:08

rpm-ostree.go: add Diff()

f44c18c

rpm-ostree.go: tweak RebaseLayered()

19c32ad

Add a comment to Rebase Don't return changed since there should be an error if nothing changed Remove logs about pivot and custom origin since we don't use pivot:// or --custom-origin-url for layered rebases

rpm-ostree.go: simplify ApplyLive()

897dd65

rpm-ostree.go: factor out Deployment struct

3519ac0

Also add LiveReplaced field A layered update will need to perform operations on the staged and deployed Deployments, which will require variables of type Deployment

mkenigs force-pushed the layering-rpm-ostree branch from 6f56f80 to 21d0c34 Compare April 1, 2022 21:08

mkenigs force-pushed the layering-rpm-ostree branch from 21d0c34 to 2445b90 Compare April 4, 2022 17:21

mkenigs added 2 commits April 4, 2022 13:17

NodeWriter: add SetLayeredDone()

b4e8699

mkenigs force-pushed the layering-rpm-ostree branch 2 times, most recently from b37bf61 to 39fd23a Compare April 4, 2022 18:43

mkenigs added 5 commits April 4, 2022 16:30

Add functionality from update to layered update

7d71461

render_controller: use MachineConfigToIgnition

405df6d

Translate MachineConfig to Ignition and pass to mco-build-content build

daemon: handle layered nodes when booting

cc33e15

Add demo hack files and instructions

8d73a1f

mkenigs force-pushed the layering-rpm-ostree branch from 39fd23a to 8d73a1f Compare April 4, 2022 21:33

openshift-ci Bot assigned jkyros Apr 5, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2022

openshift-merge-robot merged commit 151927b into openshift:layering Apr 5, 2022

mkenigs deleted the layering-rpm-ostree branch April 5, 2022 17:02

Conversation

mkenigs commented Mar 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Mar 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkenigs commented Mar 21, 2022

Uh oh!

Uh oh!

jkyros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkenigs commented Apr 4, 2022

Uh oh!

mkenigs commented Apr 4, 2022

Uh oh!

openshift-ci Bot commented Apr 4, 2022

Uh oh!

jkyros commented Apr 5, 2022

Uh oh!

openshift-ci Bot commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkyros left a comment •

edited

Loading