Add functionality from update to layered update #1

mkenigs · 2022-02-25T22:34:20Z

First 6 commits factor out code from update(). The 7th calls all the factored out code in experimentalUpdateLayeredConfig

pkg/daemon/update.go

mkenigs · 2022-02-28T15:13:42Z

pkg/daemon/update.go

Do we need to check if this is nil? Looks like we do it in setWorking but not updateConfigAndState. I think we also might need to add some more of the code from updateConfigAndState

mkenigs · 2022-02-28T15:16:42Z

@cgwalters do we need to set the scheduler to BFQ before rebasing to an image? Also why don't we currently undo setting the scheduler to BFQ? Does it reset on reboot? What about for liveapply?

cgwalters · 2022-03-01T17:41:47Z

@cgwalters do we need to set the scheduler to BFQ before rebasing to an image? Also why don't we currently undo setting the scheduler to BFQ? Does it reset on reboot? What about for liveapply?

The BFQ thing was basically a failure on my part. I think actually layering will itself help significantly here because we will avoid transiently writing large amounts of data to disk. We can probably just drop BFQ then.

Does it reset on reboot?

Yes.

jkyros · 2022-03-02T17:33:48Z

So I cleaned some stuff up into constants in update.go instead of using the raw annotations and added some SetDone calls so the pool would stay happy after rebase, but for the most part I stayed out of update.go. (You can kind of see where I was going over here, I didn't want to conflict with you

machine-config-operator/pkg/daemon/update.go

Line 2117 in 50b187d

    
           if err := dn.nodeWriter.SetDone(dn.kubeClient.CoreV1().Nodes(), dn.nodeLister, dn.name, desiredConfig); err != nil {

)

Is this in a spot where I can marge your commits commits here and then we can stitch it back together for a demo tomorrow or you want me to hold off?

mkenigs · 2022-03-03T04:55:29Z

Okay I think this should be ready to merge if you don't mind some broken things. I certainly haven't tested it. Not sure how thoroughly you can review before demoing but would love if you at least glance at the second to last commit (the one that touches experimentalUpdateLayeredConfig(). Things to keep working on:

Disable config drift monitor
Un revert your commit. The last commit I have should probably be squashed with the reverted one. I still have some questions about how some of the annotation stuff works.
(also related to the annotations) handling setting done after we reboot?
Fix unit tests

jkyros · 2022-03-03T08:12:58Z

I tried to merge it here: https://github.com/jkyros/machine-config-operator/tree/mcbs-minimize-mcd-try-merge

It built, but the cluster failed to boostrap. I will be honest though I didn't give it a ton of review before I did that -- it was more "eh, let's see what happens!". I stayed up too late -- I'll look at it tomorrow morning :)

cgwalters · 2022-03-03T22:10:20Z

Is the plan to push this to the layering branch of the MCO main git?

mkenigs · 2022-03-03T22:11:18Z

I think so

cgwalters · 2022-03-03T22:14:38Z

pkg/daemon/update.go

 			return err
 		}

-		//defer os.Unlink("/run/ostree/auth.json")


Not sure how much this matters, but remember everything in /run goes away on reboot. It may be simpler to just leave it there?

Do we want to leave it there if we apply live? Agreed probably doesn't matter much

I left it there in case I wanted to horse with it manually -- I figured if I had to "drive it out of the ditch" because I broke it, I didn't want to have to go get credentials too :)

Is there any risk to leaving credentials lying around? If not I'll take out the removal I put back in

I don't think there's a real risk here, but we should be sure the file ownership/permissions are root:root and 0600.

cgwalters · 2022-03-03T22:15:01Z

pkg/daemon/update.go

-	var targetNamespace = "openshift-machine-config-operator"
+	var targetNamespace = ctrlcommon.MCONamespace


Looks like a good cleanup we could factor out into a prep patch now and merge to main/master.

cgwalters · 2022-03-03T22:16:48Z

pkg/daemon/rpm-ostree.go

 	return rawOut, nil
 }

+type Deployment struct {


Hmm one thing we could do is get a start on coreos/rpm-ostree#2389 by extracting the current MCO code to a logically separate package (e.g. pkg/rpmostree), but keep it in the MCO git to start.

jkyros · 2022-03-04T04:01:41Z

Is the plan to push this to the layering branch of the MCO main git?

Yeah, unless you have a better idea. :)

We need somewhere where the merge/review bar is lower than our main branch and we'd be willing to tolerate the code being in some "imperfect intermediate states" on occasion -- somewhere where "I don't know that I'm totally sold on this, but it works for now" is okay.

(That, and I'd like this to be accessible to where someone can build/play in the working poc without having to build using code from 7 different open PRs because we're still haggling about the details )

I know that comes with certain risks of too many changes stacking up in the branch, etc so we'll have to pay attention and adjust if necessary.

mkenigs · 2022-03-04T14:24:49Z

I know that comes with certain risks of too many changes stacking up in the branch, etc so we'll have to pay attention and adjust if necessary.

Will we be able to repeatedly/quickly merge master into layering? E.g. if PRs like openshift#2982 or openshift#2973 go into master

cgwalters · 2022-03-04T14:57:15Z

We need somewhere where the merge/review bar is lower than our main branch and we'd be willing to tolerate the code being in some "imperfect intermediate states" on occasion -- somewhere where "I don't know that I'm totally sold on this, but it works for now" is okay.

💯 on this!

Will we be able to repeatedly/quickly merge master into layering?

I think so...if that turns out to be annoying, we will figure out how to make it less annoying.

This code will be needed for layered updates

Consolidate all code that will be needed for layered updates into calculatePostConfigChangeActionFromFiles Add a reboot file exception for ssh keys to preserve current behavior of not rebooting for passwd changes

isDrainRequired needs to support multiple types of file content, since layered updates will require ostree cat'ing a file while legacy updates read the contents directly from MachineConfigs. This is accomplished by passing isDrainRequired functions to read content from old and new files

This code will be needed for layered updates

This will keep layered and non-layered update logging consistent

Layered updates will only need to perform post config change actions. Comments about uncordoning the node and rebooting immediately were dropped. Are these comments out of date? Was code dropped or am I missing something?

Add a comment to Rebase Write pull secret to file defined in new constant ostreeAuthFile Don't return changed since there should be an error if nothing changed

mkenigs commented Feb 25, 2022

View reviewed changes

pkg/daemon/update.go Outdated Show resolved Hide resolved

mkenigs marked this pull request as draft February 25, 2022 22:47

mkenigs force-pushed the integrate-update branch from e5c9cca to 3507386 Compare February 28, 2022 15:09

mkenigs commented Feb 28, 2022

View reviewed changes

mkenigs force-pushed the integrate-update branch from 3507386 to 350dc62 Compare March 3, 2022 04:44

cgwalters reviewed Mar 3, 2022

View reviewed changes

mkenigs mentioned this pull request Mar 4, 2022

Add MCONamespace constant openshift/machine-config-operator#2982

Closed

mkenigs added 13 commits March 7, 2022 19:25

update.go: factor out setWorking()

e45771b

This code will be needed for layered updates

update.go: refactor calculatePostConfigChangeAction()

741a7cf

Consolidate all code that will be needed for layered updates into calculatePostConfigChangeActionFromFiles Add a reboot file exception for ssh keys to preserve current behavior of not rebooting for passwd changes

test calculatePostConfigChangeAction changes

66d601e

Factor out performDrain into drainIfRequired

54255bc

This code will be needed for layered updates

Move log statement to UpdateTuningArgs

77a79d1

This will keep layered and non-layered update logging consistent

update.go: split performPostConfigChangeAction

8e4a104

Layered updates will only need to perform post config change actions. Comments about uncordoning the node and rebooting immediately were dropped. Are these comments out of date? Was code dropped or am I missing something?

Add MCONamespace constant

7b2dca1

rpm-ostree.go: add Cat()

5d687c5

rpm-ostree.go: add Diff()

16bf796

rpm-ostree.go: tweak RebaseLayered()

95a1554

Add a comment to Rebase Write pull secret to file defined in new constant ostreeAuthFile Don't return changed since there should be an error if nothing changed

rpm-ostree.go: factor out logCustomOrigin()

ef7ba91

rpm-ostree.go: add ApplyLive()

7700965

mkenigs added 4 commits March 18, 2022 14:46

rpm-ostree.go: factor out Deployment struct

23430e5

Add MCONamespace constant and use in GetPullSecret

97f29ca

Add functionality from update to layered update

51139c4

Looks like we should add this for consistency before the setDone call

f28ade5

mkenigs force-pushed the integrate-update branch from 350dc62 to f28ade5 Compare March 18, 2022 19:47

mkenigs mentioned this pull request Mar 29, 2022

adds openshift image derivation test openshift/os#746

Merged

mkenigs closed this Mar 30, 2022

		var targetNamespace = "openshift-machine-config-operator"
		var targetNamespace = ctrlcommon.MCONamespace

Add functionality from update to layered update #1

Add functionality from update to layered update #1

Uh oh!

Conversation

mkenigs commented Feb 25, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkenigs commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Mar 1, 2022

Uh oh!

jkyros commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkenigs commented Mar 3, 2022

Uh oh!

jkyros commented Mar 3, 2022

Uh oh!

cgwalters commented Mar 3, 2022

Uh oh!

mkenigs commented Mar 3, 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!

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!

jkyros commented Mar 4, 2022

Uh oh!

mkenigs commented Mar 4, 2022

Uh oh!

cgwalters commented Mar 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkenigs commented Feb 28, 2022 •

edited

Loading

jkyros commented Mar 2, 2022 •

edited

Loading