Skip to content

Add registry storage configuration modules#14390

Merged
bmcelvee merged 1 commit intoopenshift:enterprise-4.1from
bmcelvee:add-registry-storage-modules
May 17, 2019
Merged

Add registry storage configuration modules#14390
bmcelvee merged 1 commit intoopenshift:enterprise-4.1from
bmcelvee:add-registry-storage-modules

Conversation

@bmcelvee
Copy link
Copy Markdown
Contributor

@bmcelvee bmcelvee commented Apr 10, 2019

@bmcelvee bmcelvee added this to the Future Release milestone Apr 10, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 10, 2019
@openshift-docs-preview-bot
Copy link
Copy Markdown

The preview will be availble shortly at:

Copy link
Copy Markdown
Contributor

@dmage dmage Apr 11, 2019

Choose a reason for hiding this comment

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

image-registry-private-configuration shouldn't be modified by users. After today's scrum I'd say there are 2 options:

  • the cloud credentials are sufficient to create an S3 bucket, in this case the operator will do the storage configuration automatically;
  • the operator is not capable of creating S3 buckets, in this case the administrator should create an S3 bucket, setup a Bucket Lifecycle Policy to abort incomplete multipart uploads that are one day old, and fill the storage configuration in configs.imageregistry.operator.openshift.io/cluster.

@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from 39145ae to 35146c6 Compare April 15, 2019 17:29
@bmcelvee bmcelvee changed the base branch from enterprise-4.0 to enterprise-4.1 April 17, 2019 14:25
@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from 35146c6 to 0495a79 Compare April 24, 2019 15:26
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 24, 2019
Copy link
Copy Markdown

@wzheng1 wzheng1 Apr 25, 2019

Choose a reason for hiding this comment

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

This command is invalid.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When UPI on AWS cluster is ready, s3 storage is automatically configured.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@wzheng1 this is only true if the cluster admin/installer provides an AWS account that has permission to create S3 buckets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should reference AWS docs on how to create an S3 bucket.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The AWS UPI doc says that you need the S3 permission to create a bucket, and the steps as written have you create a bucket to store your bootstrap ignition config.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you delete all s3 part on AWS with UPI, it will be generated automatically, admin doesn't need to input manually.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Admin may want to create a cluster with credentials that doesn't allow to create buckets. In this case the administrator should configure it manually.

Comment thread modules/registry-configuring-storage-baremetal.adoc Outdated
Comment thread modules/registry-configuring-storage-baremetal.adoc Outdated
Comment thread modules/registry-configuring-storage-vsphere.adoc Outdated
Comment thread modules/registry-configuring-storage-vsphere.adoc Outdated
@wzheng1
Copy link
Copy Markdown

wzheng1 commented Apr 25, 2019

Another thing I think need to add is to pay attention whether the cluster has default storage class, since if it has, any pvc will be attached to it automatically, this doesn't adapt to image-registry since inconsistent accessMode. So user has to specify in pv which pvc should be claimed like below:
"claimRef": {
"namespace": "openshift-image-registry",
"name": "image-registry-storage"
},
@adambkaplan please help to review too.

@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from 0495a79 to 5e2a627 Compare May 8, 2019 17:54
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@wzheng1 this is only true if the cluster admin/installer provides an AWS account that has permission to create S3 buckets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should reference AWS docs on how to create an S3 bucket.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add bucket: <bucket-name>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oc get pods -n openshift-image-registry

Comment thread modules/registry-configuring-storage-aws-upi.adoc Outdated
Comment thread modules/registry-configuring-storage-baremetal.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oc get clusteroperator image-registry

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oc get pod -n openshift-image-registry

@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from 5e2a627 to fffc92e Compare May 13, 2019 14:10
@bmcelvee
Copy link
Copy Markdown
Contributor Author

@openshift/team-documentation - please peer review

(I think the suggested PVs for bare metal and vSphere will need to change once we have storage updates, but that can come in a follow-up PR.)

@bmcelvee bmcelvee added the peer-review-needed Signifies that the peer review team needs to review this PR label May 13, 2019
@bmcelvee bmcelvee changed the title [WIP] Add registry storage configuration modules Add registry storage configuration modules May 13, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2019
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

an S3 bucket,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The AWS UPI doc says that you need the S3 permission to create a bucket, and the steps as written have you create a bucket to store your bootstrap ignition config.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@huffmanca, when you're back in the office, we need to talk about the install-level prereqs we need to state for storage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd move "such as Azure File or NFS." info to the prereq.

Can you show the commands that you'd run for these two steps?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What am I looking for here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$oc edit configs.imageregistry.operator.openshift.io
storage:
pvc:
claim:

If you leave the claim name to be blank, a pvc named image-registry-storage will be created automatically.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you change the registry storage after installation? Do they need to go here, or just in the installation assemblies?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, you can, as long as you are admin : ) vsphere and baremetal UPI installation should configure storage after installation, as original installation will prompt storage not configured error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would you use Azure or NFS with vSphere? I'd still move this info to the prereqs and show the commands to do these first two steps.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes it look like your storage is still not configured. Is that right? If you wait and run the command again, should you see a different message?

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels May 13, 2019
@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from fffc92e to 4d5cf25 Compare May 13, 2019 20:29
@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from 4d5cf25 to 04ce691 Compare May 14, 2019 13:54
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adambkaplan @dmage - would either of you be able to provide the procedures for these two steps or point me to where I can find them? Thanks!

. To configure your registry to use storage, change the spec.storage.pvc in
the configs.imageregistry/cluster resource.

. Provide a suitable persistent volume.

Copy link
Copy Markdown
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

@bmcelvee the steps here don't really make sense to me. Configuring PVC storage is much simpler:

  1. Cluster admin must set up Persistent Volume(s) that support ReadWriteMany access modes (ex: NFS) - prerequisite and out of scope.
  2. Make sure the registry is not up - check status of the cluster operator or operator config:
    $ oc get clusteroperator image-registry or $ oc get config.imageregistry.operator.openshift.io/cluster -o yaml
  3. Edit the cluster operator config to set the storage to use PVC
  4. Wait for the registry to come up (watch the clusteroperator - should report Available=true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* PV with `ReadWriteMany` access mode, such as Azure File or NFS.
* A provisioned Persistent Volume with `ReadWriteMany` access mode, such as Azure File or NFS.

You may also want out-link to upstream docs on PVs: https://v1-13.docs.kubernetes.io/docs/concepts/storage/persistent-volumes/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bmcelvee I'd consider this out of scope for this portion of the doc - the assumed prerequisite is that the cluster admin provisioned storage that can be satisfied via a PVC.

@bmcelvee bmcelvee force-pushed the add-registry-storage-modules branch from 04ce691 to 749b501 Compare May 17, 2019 17:42
@bmcelvee
Copy link
Copy Markdown
Contributor Author

I think we've reached a point where this content can be merged. If it requires additional content or changes, those items can be addressed in a follow-up PR.

@bmcelvee bmcelvee merged commit 0c92649 into openshift:enterprise-4.1 May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.1 peer-review-done Signifies that the peer review team has reviewed this PR 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.

8 participants