Bug 1918440: daemon: add check before updating kernelArgs #2349

sinnykumari · 2021-01-20T11:07:42Z

Today we are unnecessarily calling updateKernelArguments() even when there is no kargs to apply. This doesn't fixes bug 1916169 but discovered in https://bugzilla.redhat.com/show_bug.cgi?id=1916169#c8.
If needed, can file a new bug but for now getting it in as part of BZ#1916169

Update: Created a new bug to track it separately.

openshift-ci-robot · 2021-01-20T11:08:02Z

@sinnykumari: This pull request references Bugzilla bug 1916169, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1916169: daemon: add check before updating kernelArgs

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.

openshift-ci-robot · 2021-01-20T11:17:07Z

@sinnykumari: This pull request references Bugzilla bug 1916169, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1916169: daemon: add check before updating kernelArgs

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.

darkmuggle · 2021-01-20T15:35:17Z

pkg/daemon/update.go

@@ -451,8 +451,10 @@ func (dn *Daemon) applyOSChanges(oldConfig, newConfig *mcfgv1.MachineConfig) (re
 	}()

 	// Apply kargs
-	if err := dn.updateKernelArguments(oldConfig, newConfig); err != nil {
-		return err
+	if mcDiff.kargs {


Doesn't dn.updateKerenlArguments do a check already?

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

Lines 1018 to 1021 in 82929b7

diff := generateKargsCommand(oldConfig, newConfig)

if len(diff) == 0 {

return nil

}

not really, generateKargsCommand() function just deletes all kargs available in old Rendered MC and append all from new rendered MC. variable name is a bit misleading, perhaps it is hanging around from our earlier implementation. We have simplified kargs logic over time due to various bugs we hit, see https://github.com/openshift/machine-config-operator/pull/2014/files . I will update variable name so that we don't get confused

Ah, okay.... let me know when you're ready for another review.

darkmuggle

/lgtm
/approve

openshift-ci-robot · 2021-01-20T17:41:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkmuggle, sinnykumari

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~~ [darkmuggle,sinnykumari]

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

openshift-ci-robot · 2021-01-20T17:50:45Z

@sinnykumari: This pull request references Bugzilla bug 1918440, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1918440: daemon: add check before updating kernelArgs

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.

openshift-ci-robot · 2021-01-20T17:51:15Z

@sinnykumari: This pull request references Bugzilla bug 1918440, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1918440: daemon: add check before updating kernelArgs

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.

openshift-bot · 2021-01-20T18:04:47Z

/retest

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

openshift-bot · 2021-01-20T18:17:47Z

/retest

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

openshift-bot · 2021-01-20T20:01:49Z

/retest

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

openshift-ci-robot · 2021-01-21T01:27:28Z

@sinnykumari: All pull requests linked via external trackers have merged:

openshift/machine-config-operator#2349

Bugzilla bug 1918440 has been moved to the MODIFIED state.

In response to this:

Bug 1918440: daemon: add check before updating kernelArgs

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.

sinnykumari · 2021-08-17T14:51:02Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-08-17T14:51:43Z

@sinnykumari: #2349 failed to apply on top of branch "release-4.6":

Applying: daemon: add check before updating kernelArgs
Applying: daemon: make kargs variable and function logical to confusion
Using index info to reconstruct a base tree...
M	pkg/daemon/update.go
M	pkg/daemon/update_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/daemon/update_test.go
Auto-merging pkg/daemon/update.go
CONFLICT (content): Merge conflict in pkg/daemon/update.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 daemon: make kargs variable and function logical to confusion
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.6

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.

Also, make kargs variable and function name logical to avoid confusion. Cherry-pick of openshift#2349

daemon: add check before updating kernelArgs

6422c45

sinnykumari changed the title ~~daemon: add check before updating kernelArgs~~ Bug 1916169: daemon: add check before updating kernelArgs Jan 20, 2021

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 20, 2021

openshift-ci-robot requested review from darkmuggle and kikisdeliveryservice January 20, 2021 11:08

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2021

sinnykumari mentioned this pull request Jan 20, 2021

Refactor kernel argument handling #2348

Closed

darkmuggle reviewed Jan 20, 2021

View reviewed changes

daemon: make kargs variable and function logical to confusion

5c6b7ab

darkmuggle approved these changes Jan 20, 2021

View reviewed changes

openshift-ci-robot assigned darkmuggle Jan 20, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021

sinnykumari changed the title ~~Bug 1916169: daemon: add check before updating kernelArgs~~ Bug 1918440: daemon: add check before updating kernelArgs Jan 20, 2021

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 20, 2021

openshift-merge-robot merged commit 40d8e4f into openshift:master Jan 21, 2021

sinnykumari added a commit to sinnykumari/machine-config-operator that referenced this pull request Aug 18, 2021

daemon: add check before updating kernelArgs

a4309c9

Also, make kargs variable and function name logical to avoid confusion. Cherry-pick of openshift#2349

sinnykumari mentioned this pull request Aug 18, 2021

[release-4.6] Bug 1989403: daemon: add check before updating kernelArgs #2720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1918440: daemon: add check before updating kernelArgs #2349

Bug 1918440: daemon: add check before updating kernelArgs #2349

sinnykumari commented Jan 20, 2021 •

edited

openshift-ci-robot commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

darkmuggle Jan 20, 2021

sinnykumari Jan 20, 2021 •

edited

darkmuggle Jan 20, 2021

sinnykumari Jan 20, 2021

darkmuggle left a comment

openshift-ci-robot commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

openshift-bot commented Jan 20, 2021

openshift-bot commented Jan 20, 2021

openshift-bot commented Jan 20, 2021

openshift-ci-robot commented Jan 21, 2021

sinnykumari commented Aug 17, 2021

openshift-cherrypick-robot commented Aug 17, 2021

	diff := generateKargsCommand(oldConfig, newConfig)
	if len(diff) == 0 {
	return nil
	}

Bug 1918440: daemon: add check before updating kernelArgs #2349

Bug 1918440: daemon: add check before updating kernelArgs #2349

Conversation

sinnykumari commented Jan 20, 2021 • edited

openshift-ci-robot commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

darkmuggle Jan 20, 2021

Choose a reason for hiding this comment

sinnykumari Jan 20, 2021 • edited

Choose a reason for hiding this comment

darkmuggle Jan 20, 2021

Choose a reason for hiding this comment

sinnykumari Jan 20, 2021

Choose a reason for hiding this comment

darkmuggle left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

openshift-ci-robot commented Jan 20, 2021

openshift-bot commented Jan 20, 2021

openshift-bot commented Jan 20, 2021

openshift-bot commented Jan 20, 2021

openshift-ci-robot commented Jan 21, 2021

sinnykumari commented Aug 17, 2021

openshift-cherrypick-robot commented Aug 17, 2021

sinnykumari commented Jan 20, 2021 •

edited

sinnykumari Jan 20, 2021 •

edited