-
Notifications
You must be signed in to change notification settings - Fork 53
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
Create MicroShift iso using image mode and bootc image builder #999
Conversation
Skipping CI for Draft Pull Request. |
6ab8c1e
to
c074a21
Compare
/test all |
125c5ac
to
83bd510
Compare
/test e2e-microshift |
3610d4d
to
fd6b49f
Compare
/test e2e-microshift |
/cherry-pick release-4.18 |
@praveenkumar: once the present PR merges, I will cherry-pick it on top of In response to this:
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-sigs/prow repository. |
fd6b49f
to
8e204a4
Compare
/hold |
8e204a4
to
2c867a0
Compare
/unhold |
firewall-offline-cmd --zone=trusted --add-source=169.254.169.1 | ||
|
||
|
||
# Configure systemd journal service to persist logs between boots and limit their size to 1G |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1G is quite big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think would be best 200M ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could set the storage as persistent but keep the default values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is now outdated though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this complete section because we don't need to persist logs between the boots since, this is something required for prod kind of setup.
# This is required to update the gpgcheck for repoID | ||
repoID=$(echo "${MIRROR_REPO#*://}" | tr '/:' '_'); \ | ||
dnf config-manager --add-repo "${MIRROR_REPO}" \ | ||
--add-repo "https://mirror.openshift.com/pub/openshift-v4/$(uname -m)/dependencies/rpms/${USHIFT_VER}-el9-beta" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this https://mirror.openshift.com/pub/openshift-v4/...
repo need to be hardcoded in addition to the user-specified "${MIRROR_REPO}"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is dependency repo and it is different from mirror repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with --use-mirror-repo
documentation (Use mirror repo to get release candidate and engineering preview rpms
) it's not immediatly obvious to me why we would force the use of ${USHIFT_VER}-el9-beta
instead of a non-beta repo.
Side-note, if --use-mirror-repo
is only meant for release candidate and engineering preview, the name is not great as it's too generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find a good name around but adding the comment why this dependencies repo is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe unreleased-mirror
or such? I'm worried the use of -beta
is unexpected in some cases, and is really non obvious from the option name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used unreleased-mirror
as environment variable to avoid confusion.
2c867a0
to
f051c02
Compare
RUN firewall-offline-cmd --zone=public --add-port=80/tcp && \ | ||
firewall-offline-cmd --zone=public --add-port=443/tcp && \ | ||
firewall-offline-cmd --zone=public --add-port=30000-32767/tcp && \ | ||
firewall-offline-cmd --zone=public --add-port=30000-32767/udp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we disable firewall in createdisk.sh
for microshift bundles, this seems redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removing this from container file.
firewall-offline-cmd --zone=trusted --add-source=169.254.169.1 | ||
|
||
|
||
# Configure systemd journal service to persist logs between boots and limit their size to 1G |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is now outdated though.
As per doc https://github.com/openshift/microshift/blob/main/docs/user/howto_config.md#drop-in-configuration-directory it is better to use the drop in configuration directory then changing the default one.
dcac478
to
febf74c
Compare
ci_microshift.sh
Outdated
@@ -5,6 +5,10 @@ set -exuo pipefail | |||
sudo yum install -y make golang | |||
|
|||
./shellcheck.sh | |||
ARCH=$(uname -m) | |||
if [[ "$ARCH" == "aarch64" ]]; then | |||
export SNC_NON_FATAL_PREFLIGHT_CHECKS=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would make more sense to fix the capabilities check on aarch64. I don't think this is directly related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not directly related to this PR but I wanted to make sure both arm and amd build succeed before merge it.
fix the capabilities check on aarch64.
Yes, I think GCP doesn't allow native nested virtualization for arm64 and current job is using emulation only and it is slow but at least we are getting bundle build for arm64, will check if some different cloud provider can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to allow emulation on a specific platform, it should be the script starting snc on this platform which sets this variable. Ideally we'd have a way to only disable the "kvm" check there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we'd have a way to only disable the "kvm" check there.
@cfergeau yes, something like SNC_DISABLE_KVM_CHECK
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping this commit as of now and going with merge, will create follow up PR for it.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau 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 |
/retest |
1 similar comment
/retest |
@praveenkumar: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
With 4.18 microshift removed the steps of creating the iso using image builder and there is no more `build.sh` script which is consumed by mircoshift.sh script to create it. This PR use the image mode and bootc image builder (BIB) to create the iso which is now microshift team also pushing forward.
febf74c
to
1df1ccb
Compare
New changes are detected. LGTM label has been removed. |
/cherry-pick release-4.18 |
@praveenkumar: #999 failed to apply on top of branch "release-4.18":
In response to this:
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-sigs/prow repository. |
@praveenkumar: #999 failed to apply on top of branch "release-4.18":
In response to this:
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-sigs/prow repository. |
With 4.18 microshift removed the steps of creating the iso using image builder and there is no more
build.sh
script which is consumed by mircoshift.sh script to create it. This PR use the image mode and bootc image builder (BIB) to create the iso which is now microshift team also pushing forward.