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

Add RHCOS kernel options before installation #129

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

aishwaryabk
Copy link
Collaborator

Signed-off-by: Aishwarya Kamat aishwarya.kamat@ibm.com

Signed-off-by: Aishwarya Kamat <aishwarya.kamat@ibm.com>
Copy link
Collaborator

@Prajyot-Parab Prajyot-Parab left a comment

Choose a reason for hiding this comment

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

/lgtm

@pravin-dsilva
Copy link
Collaborator

/lgtm

@Prajyot-Parab Prajyot-Parab requested a review from bpradipt August 11, 2021 09:07
@yussufsh
Copy link
Contributor

yussufsh commented Sep 2, 2021

IMO we should use existing rhcos_kernel_options itself in the manifest file as day 1 operation. I don't think there is any harm in also keeping it in the post install steps.
Any comments @Prajyot-Parab @bpradipt @pravin-dsilva ?

@@ -48,6 +48,7 @@ workdir: <Directory to use for creating OCP configs>
storage_type: <Storage type used in the cluster. Eg: nfs (Note: Currently NFS provisioner is not configured using this playbook. This variable is only used for setting up image registry to EmptyDir if storage_type is not nfs)>
log_level: <Option --log-level in openshift-install commands. Default is 'info'>
release_image_override: '<This is set to OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE while creating ign files. If you are using internal artifactory then ensure that you have added auth key to the pull_secret>'
rhcos_pre_kernel_options: <List of day-1 kernel options for RHCOS nodes eg: ["rd.multipath=default","root=/dev/disk/by-label/dm-mpath-root"]>
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR looks good, but I'm wondering if we can come up with a better var name ?

Copy link
Contributor

Choose a reason for hiding this comment

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

eg. rhcos_day1_kernel_options ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we should add everything from rhcos_kernel_options in the manifest. No need of a new variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per my understanding, we can use the same existing var but then we would need logic to decide when to use it either pre or post installation. If that adds additional var then we can stick to current PR changes itself.

IMO we can stick to current approach with renaming the vars as it would be much simpler and easier to understand.

^^^ @aishwaryabk @pravin-dsilva

@Prajyot-Parab Prajyot-Parab merged commit 467ffb6 into ocp-power-automation:master Sep 22, 2021
@yussufsh
Copy link
Contributor

@aishwaryabk @Prajyot-Parab ensure this is backported in the release branches.
Do we need a branch for 4.8?

@aishwaryabk
Copy link
Collaborator Author

@yussufsh Thanks! Backported in the release branches - 4.5, 4.6 and 4.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants