8000 config/fcos/v1_7_exp/translate.go: added required kernel arguments to… by madhu-pillai · Pull Request #610 · coreos/butane · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

config/fcos/v1_7_exp/translate.go: added required kernel arguments to… #610

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

config/fcos/v1_7_exp/translate.go: added required kernel arguments to… #610

wants to merge 1 commit into from

Conversation

madhu-pillai
Copy link
Contributor

Added the required kernelArguments to unlock the device in the s390x sugar.

[root@a3elp53 butane]# cat bin/s390x/fcos.yaml 
variant: fcos
version: 1.7.0-experimental
boot_device:
  layout: s390x-eckd
  luks:
    device: /dev/dasda
    cex:
      enabled: true

[root@a3elp53 s390x]# ./butane fcos.yaml -o fcos.ign
[root@a3elp53 s390x]# cat fcos.ign | jq '.'
{
  "ignition": {
    "version": "3.6.0-experimental"
  },
  "kernelArguments": {
    "shouldExist": [
      "rd.luks.key=/etc/luks/cex.key"
    ]
  },
  "storage": {
    "filesystems": [
      {
        "device": "/dev/mapper/root",
        "format": "xfs",
        "label": "root",
        "wipeFilesystem": true
      }
    ],
    "luks": [
      {
        "cex": {
          "enabled": true
        },
        "device": "/dev/dasda2",
        "label": "luks-root",
        "name": "root",
        "wipeVolume": true
      }
    ]
  }
}

@madhu-pillai madhu-pillai mentioned this pull request May 19, 2025
Merged
prestist
prestist reviewed May 20, 2025
View reviewed changes
Copy link
Collaborator
@prestist prestist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this, and getting these changes in so quickly!
The changes make sense, I just have a few nits/questions.

The commit message exceeds 80 character limit, would you mind making it more concise to abide by that and expand on details in a new line under the summary if necessary?

Additionally can we add a test verifying the sugar's functionality in the translation tests?

docs/release-notes.md Outdated
@@ -9,6 +9,7 @@ nav_order: 9
### Features

- Validate merged/replaced Ignition configs if they are local/inline _(all base specifications)_
- Added Kernel Argument rd.luks.keys in s390x layout sugar.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong tense, always present not past

"Expand existing CEX sugar to include a required kernel arg"

config/fcos/v1_7_exp/translate.go Outdated
// Add Kernel Arguments for CEX key file
rendered.KernelArguments = types.KernelArguments{
ShouldExist: []types.KernelArgument{
types.KernelArgument("rd.luks.key=/etc/luks/cex.key"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be 100% sure, the path will always be the same? no chance it would vary between systems?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, it's hardcoded here: https://github.com/coreos/fedora-coreos-config/pull/2986/files#diff-c526dfd1e153e193423394e0ed643fb22184d4d45167d1737754e8564c3f5becR141

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/coreos/ignition/pull/1820/files#diff-01035e9282707d3e15d0f2f82a9869e11b017833be34e0589dab4c5282b708c2

@travier
Copy link
Member
travier commented May 20, 2025

#605 (comment)

@travier
Copy link
Member
travier commented May 20, 2025

I think it's too late to change this now. We should add validation that makes sure that the karg exists and is set to the correct value instead.

@travier
Copy link
Member
travier commented May 20, 2025

As this would be validation, this does not block the 4.19 spec stabilisation and can happen later.

@travier travier mentioned this pull request May 20, 2025
Open
@travier
Copy link
Member
travier commented May 20, 2025

#611

@prestist
Copy link
Collaborator

@travier

I think it's too late to change this now. We should add validation that makes sure that the karg exists and is set to the correct value instead.

100% agree

@travier
Copy link
Member
travier commented May 21, 2025

Closing this one as we'll do validation instea: #611

@travier travier closed this May 21, 2025
@travier travier reopened this May 21, 2025
@travier
Copy link
Member
travier commented May 21, 2025

I'm re-opening this one as we'll have to edit the OCP docs anyway to fix other things, so we might as well get this one in and re-do all the testing.

@travier travier mentioned this pull request May 21, 2025
Open
@travier
Copy link
Member
travier commented May 21, 2025

Turns out we should not just add this as a karg for the openshift case as that would add an unnecessary reboot, so we need to special case the openshift translation. See: #613

@travier travier mentioned this pull request May 21, 2025
Open
41 tasks
@travier
Copy link
Member
travier commented May 22, 2025

I tried working on this a bit and the more it goes, the less I like it. So let's go back to what we had initially decided in #610 (comment) and do validation only: #611

@travier travier closed this May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prestist prestist prestist left review comments

@travier travier travier left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@madhu-pillai @travier @prestist
0