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

ztp: Add a local registry to the SNO DU profile (CNF-3759) #939

Conversation

pixelsoccupied
Copy link
Contributor

A SNO DU can have limited BW between the centralized registry and itself. This can lead to long image download times. This is unavoidable for the initial deployment. For subsequent pulls of an image we are relying on the fact that the image would be in the cache. There is the possibility that it could have been evicted but this would be the exception vs the rule.

Unfortunately this strategy is flawed as crio will wipe /var/lib/containers/storage in the case on an unclean shutdown

To address this, a local registry will be required on the SNO. The cluster images would need to be mirrored to the this registry along with being used as the regisrty for the applications.

@openshift-ci openshift-ci bot requested review from marioferh and serngawy February 2, 2022 18:31
@pixelsoccupied
Copy link
Contributor Author

/cc @browsell @imiller0 @lack

@openshift-ci openshift-ci bot requested review from browsell, imiller0 and lack February 2, 2022 18:32
@pixelsoccupied
Copy link
Contributor Author

pixelsoccupied commented Feb 2, 2022

Let me know if there's a better place to put these file in.

@browsell
Copy link

We want these CR's to be generated automatically by siteconfig and applied as part of the installation. The siteconfig should take the device and partition size as parameters and generate the required CRs

@pixelsoccupied
Copy link
Contributor Author

pixelsoccupied commented Feb 10, 2022

Ah so something like how we generate CRs for say BMH? So that would be a new feature of the code base that parses the SiteConfig CR?

@lack
Copy link
Member

lack commented Feb 11, 2022

Basically, expand the SiteConfig CRD with a way to specify additional disk partitions.

Question: for standard clusters, would a customer want to specify different partitioning for masters vs workers?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2022
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch 2 times, most recently from 51a3cae to f709151 Compare February 21, 2022 21:07
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2022
@pixelsoccupied
Copy link
Contributor Author

Going to remove the code and rely on templating instead

@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 23, 2022
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from f38b201 to a89107a Compare February 23, 2022 18:23
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2022
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from a89107a to 842b729 Compare February 23, 2022 19:40
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2022
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch 4 times, most recently from 435f28e to 6899c60 Compare February 23, 2022 20:34
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from 6a833b2 to 0fb905f Compare February 25, 2022 21:16
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from ac95962 to 568ec51 Compare March 15, 2022 18:49
@lack
Copy link
Member

lack commented Mar 23, 2022

My only other comment, besides the question about the go module changes, would be that we should streamline the git history here before it's merged too, please!

go.mod Outdated Show resolved Hide resolved
ztp/image-registry-crs/README.md Outdated Show resolved Hide resolved
ztp/image-registry-crs/README.md Outdated Show resolved Hide resolved
ztp/image-registry-crs/README.md Outdated Show resolved Hide resolved
ztp/source-crs/ImageRegistryPvc.yaml Outdated Show resolved Hide resolved
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from c4d5b0b to 814a4fe Compare March 28, 2022 13:55
@pixelsoccupied
Copy link
Contributor Author

/retest-required

@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from cc59981 to 688f59c Compare March 28, 2022 15:37
ztp: update readme with extra info about verfication

ztp: move pv and pvc create SourceCR and update README

ztp: init commit to auto generate mc for image registry partition

ztp: clean up

ztp: remove code used to generate machineconfig and replace with templating for consistency

ztp: clean up

ztp: remove or revert unused files or changes

ztp: revert changes

ztp: fix formatting of siteConfig

ztp: update go.mod with vendor

ztp: add ztp wave annotation for the pv and pvs in SourceCR

ztp: split SourceCRs, use mount_mount instead of lable for user input, better error handling and error msg with solutions

ztp: fix test case and error message

ztp: remove prepending var to path

ztp: check for missing mount point before processing it

ztp: add tests and clean up the code with correct variable names

ztp: include updated mod file

ztp: update test cases and better error handling

ztp: fix formatting

ztp: add a warning on the generated CR

Update ztp/image-registry-crs/README.md

Co-authored-by: Ian Miller <[email protected]>

ztp: use existing src cr pvc, update doc, use latest coreos library instead v22, add sources files under pgt kuztomize test

ztp: remove extra lines

ztp: rename filename to keep things consistent

ztp: enable dynamic update of disk format and a todo for encryption

ztp: update doc to include siteConfig values
@pixelsoccupied pixelsoccupied force-pushed the add-local-image-registry branch from 2fa75ed to ff9ff55 Compare March 28, 2022 18:19
@pixelsoccupied pixelsoccupied requested review from lack and imiller0 March 28, 2022 19:42
@lack
Copy link
Member

lack commented Mar 29, 2022

/lgtm
/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022
@imiller0
Copy link
Contributor

/retest

@lack
Copy link
Member

lack commented Mar 30, 2022

/override ci/prow/e2e-gcp-ovn

These changes do not have anything to do with SRO which is causing the failures.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 30, 2022

@lack: Overrode contexts on behalf of lack: ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-gcp-ovn

These changes do not have anything to do with SRO which is causing the failures.

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.

@lack
Copy link
Member

lack commented Mar 30, 2022

/approve

@lack
Copy link
Member

lack commented Mar 30, 2022

Ah, we need approval from someone on the go.mod changes too before this can merge.

@pixelsoccupied
Copy link
Contributor Author

/assign @fromanirh

@ffromani
Copy link
Member

/approve
the go.mod changes seems straightforward
trusting you folks for the content review

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fromanirh, lack, pixelsoccupied

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit d67b42b into openshift-kni:master Mar 30, 2022
@pixelsoccupied
Copy link
Contributor Author

/cherrypick 4.10

@openshift-cherrypick-robot

@pixelsoccupied: cannot checkout 4.10: error checking out 4.10: exit status 1. output: error: pathspec '4.10' did not match any file(s) known to git

In response to this:

/cherrypick 4.10

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.

@pixelsoccupied
Copy link
Contributor Author

/cherrypick release-4.10

@openshift-cherrypick-robot

@pixelsoccupied: #939 failed to apply on top of branch "release-4.10":

Applying: ztp: Add a local registry to the SNO DU profile (CNF-3759)
.git/rebase-apply/patch:65: trailing whitespace.
   
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	go.mod
M	ztp/policygenerator-kustomize-plugin/testPolicyGenTemplate/site-du-sno-1-ranGen.yaml
M	ztp/siteconfig-generator-kustomize-plugin/testSiteConfig/site1-sno-du.yaml
M	ztp/siteconfig-generator/siteConfig/siteConfig.go
M	ztp/siteconfig-generator/siteConfig/siteConfigBuilder_test.go
M	ztp/siteconfig-generator/siteConfig/siteConfig_test.go
M	ztp/siteconfig-generator/siteConfig/testdata/siteConfigTestOutput.yaml
Falling back to patching base and 3-way merge...
Auto-merging ztp/siteconfig-generator/siteConfig/testdata/siteConfigTestOutput.yaml
CONFLICT (content): Merge conflict in ztp/siteconfig-generator/siteConfig/testdata/siteConfigTestOutput.yaml
Auto-merging ztp/siteconfig-generator/siteConfig/siteConfig_test.go
CONFLICT (content): Merge conflict in ztp/siteconfig-generator/siteConfig/siteConfig_test.go
Auto-merging ztp/siteconfig-generator/siteConfig/siteConfigBuilder_test.go
Auto-merging ztp/siteconfig-generator/siteConfig/siteConfig.go
CONFLICT (content): Merge conflict in ztp/siteconfig-generator/siteConfig/siteConfig.go
Auto-merging ztp/siteconfig-generator-kustomize-plugin/testSiteConfig/site1-sno-du.yaml
Auto-merging ztp/policygenerator-kustomize-plugin/testPolicyGenTemplate/site-du-sno-1-ranGen.yaml
CONFLICT (content): Merge conflict in ztp/policygenerator-kustomize-plugin/testPolicyGenTemplate/site-du-sno-1-ranGen.yaml
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 ztp: Add a local registry to the SNO DU profile (CNF-3759)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-4.10

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants