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 RHEL support for Nutanix to image-builder #2558

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

adiantum
Copy link
Contributor

Description of changes:

Add RHEL8 and RHEL9 images support for Nutanix

  • change AWS image-builder to support RHEL8 and RHEL9 for Nutanix
  • add patches for K8S image-builder
  • revert Makefile changes for nutanix config: the changes break connection to nutanix cluster

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

 - add RHEL8 and RHEL9 support for Nutanix
@eks-distro-bot
Copy link
Collaborator

Hi @adiantum. Thanks for your PR.

I'm waiting for a aws member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@eks-distro-bot eks-distro-bot added needs-ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2023
@jaxesn
Copy link
Member

jaxesn commented Oct 16, 2023

I'll review this today


PACKER_NUTANIX_CONF_FILES=$(MAKE_ROOT)/packer/nutanix/nutanix.json
PACKER_NUTANIX_VAR_FILES+=$(PACKER_NUTANIX_CONF_FILES)
PACKER_NUTANIX_VAR_FILES?=
Copy link
Member

Choose a reason for hiding this comment

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

we made this change after bumping to the latest image-builder release tag because it started validated the nutanix settings. I was curious about why this started and if this change would cause issues. How are you avoiding the validation error now without this change?

Copy link
Contributor Author

@adiantum adiantum Oct 16, 2023

Choose a reason for hiding this comment

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

We're providing nutanix settings to AWS image-builder with --nutanix-config option. nutanix-config is json file with the same params like in nutanix.json

For example:

$ cat ./nutanix-connection-rhel-9.json
{
  "nutanix_cluster_name": "e2e***",
  "source_image_name": "rhel-9.2-x86_64-kvm.qcow2",
  "image_url": "http://data.***/rhel-9.2-x86_64-kvm.qcow2",
  "image_name": "eksa-rhel-9-kube-1-27",
  "nutanix_subnet_name": "vlan***",
  "nutanix_endpoint": "prism-ncn-ci.***",
  "nutanix_insecure": "false",
  "nutanix_port": "9440",
  "nutanix_username": "***",
  "nutanix_password": "***",
  "rhel_username": "***",
  "rhel_password": "****"
}

JFYI: I've tested it with the following command:

CODEBUILD_CI=true CODEBUILD_SRC_DIR=/***/eks-anywhere-build-tooling REPO_NO_CLONE=true ./image-builder build --os redhat --os-version 9 --hypervisor nutanix --release-channel 1-27 --nutanix-config ./nutanix-connection-rhel-9.json -v 20

Copy link
Member

Choose a reason for hiding this comment

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

yeah thats kind of what I figured. Its possible its just a presubmit issue. let me enable the tests and lets see what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's failed on presubmit, but if we don't revert it then image-builder failed to work with nutanix clusters. I have an idea how to fix it, return in short time after testing both cases.

@jaxesn
Copy link
Member

jaxesn commented Oct 16, 2023

/ok-to-test

fmt.Sprintf("%s=%s", rhelUsernameEnvVar, b.NutanixConfig.RhelUsername),
fmt.Sprintf("%s=%s", rhelPasswordEnvVar, b.NutanixConfig.RhelPassword),
fmt.Sprintf("%s=%s", rhelImageUrlNutanixEnvVar, b.NutanixConfig.ImageUrl),
fmt.Sprintf("%s=%s", "PACKER_NUTANIX_TEST_CONF_FILES", " "),
Copy link
Member

Choose a reason for hiding this comment

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

im thinking maybe something like this instead?

https://github.com/aws/eks-anywhere-build-tooling/blob/main/projects/aws/image-builder/builder/builder.go#L173C65-L173C89

thats what we do for cloudstack and baremetal. cc @abhay-krishna

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaxesn I've changed it in similar way like cloudstack and baremetal.

Copy link
Member

@jaxesn jaxesn left a comment

Choose a reason for hiding this comment

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

Thanks @adiantum for the contribution!

/approve

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaxesn

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

@eks-distro-bot eks-distro-bot merged commit d6c78fe into aws:main Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants