Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kargs: Support --append and --delete simultaneously #1940

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

cgwalters
Copy link
Member

Code I wrote for the machine-config-operator expected it to
work, and I don't see a reason not to support it.

See openshift/machine-config-operator#1265

Code I wrote for the machine-config-operator expected it to
work, and I don't see a reason not to support it.

See openshift/machine-config-operator#1265
@lucab
Copy link
Contributor

lucab commented Nov 17, 2019

@cgwalters this PR itself looks fine to me, however I have some concerns on the MCO logic.

In particular, I fear that a delete+append will end up re-ordering the kargs as a side-effect.

In some cases (like hugepages) the relative order between arguments seems to be relevant, see https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/virtualization_tuning_and_optimization_guide/sect-virtualization_tuning_optimization_guide-memory-huge_pages-1gb-runtime.

@cynepco3hahue
Copy link

@lucab I can see that rpm-ostree kargs order kernel arguments by default, that incorrect, for example for hugepages you can specify different sizes of huge pages on boot time.
hugepagesz=1G hugepages=4 hugepagesz=2M hugepages=1024 in this case it should allocate 4 1Gb hugepages and 1024 2M hugepages, but when I pass it to the rpm-ostree kargs --append=hugepagesz=1G --append=hugepages=4 --append=hugepagesz=2M --append=hugepages=1024 after the reboot I got:

# cat /proc/cmdline
... console=tty0 console=ttyS0,115200n8 rd.luks.options=discard ostree=/ostree/boot.1/rhcos/73879866f874786fe64bcd407218b9ba9045c8b5a19078e0e467e0e5481ce5ed/0 ignition.platform.id=openstack hugepagesz=1G hugepagesz=2M hugepages=4 hugepages=1024

So this PR will not ruin the order, because it already ruined, rpm-ostree should preserve the order of parameters.

@cgwalters
Copy link
Member Author

See ostreedev/ostree#1859 for ordering.

@cgwalters
Copy link
Member Author

I think clearly in the general case what the MCO really wants to do is be "stateless" by simply resetting the kernel args to default, then appending the ones it wants. I.e. it should work the same way that ostree handles package layering (check out pristine new FS, apply changes).

However, this gets into the issue of "default" kargs ostreedev/ostree#479

@cgwalters
Copy link
Member Author

cgwalters commented Nov 21, 2019

See ostreedev/ostree#1859 for ordering.

Which isn't in a libostree release yet; need to do one and get it out there, and then we can verify that the rpm-ostree side kargs logic all works with this too.

That said, I agree with

So this PR will not ruin the order, because it already ruined,

Let's merge this to fix up the MCO, then revisit the more general problem later, in particular default kargs?

@cynepco3hahue
Copy link

@cgwalters A lot of thanks for the information, when can we expect a new release of libostree?

@lucab
Copy link
Contributor

lucab commented Nov 21, 2019

/lgtm

@jlebon
Copy link
Member

jlebon commented Nov 21, 2019

/lgtm
too

That said, looks like we have some CI issues:

Error: 
 Problem 1: conflicting requests
  - nothing provides librpm.so.8()(64bit) needed by rpm-ostree-6b21e30f-1.fc30.x86_64
  - nothing provides librpmio.so.8()(64bit) needed by rpm-ostree-6b21e30f-1.fc30.x86_64

Ahh OK, looks like the cosa buildroot hasn't been updated in a while so it's still at f30. I started a new build now. Seems like the trigger isn't working.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon, lucab

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:

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

@jlebon
Copy link
Member

jlebon commented Nov 21, 2019

@cgwalters
Copy link
Member Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit f295f54 into coreos:master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants