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.

Sign up for GitHub

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

Jump to bottom

Move steps for migrating code to csi-operator to ocp enhancements #1644

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Move steps for migrating code to csi-operator to ocp enhancements #1644

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1905962
Move steps for migrating code to csi-operator to ocp enhancementsn
gnufied Jun 13, 2024
d0883f9
Add a note about ocp-build-data changes to be merged same time as csi…
gnufied Jul 1, 2024
e7d5e02
Add steps for post merge cleanup
gnufied Jul 1, 2024
4e0d5bd
Update enhancements/storage/csi-driver-operator-merge.md
gnufied Jul 2, 2024
75ef547
Update enhancements/storage/csi-driver-operator-merge.md
gnufied Jul 2, 2024
9660e8c
Add additional docs for OLM based operators
gnufied Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 75 additions & 1 deletion enhancements/storage/csi-driver-operator-merge.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,80 @@ N/A

Same as today.

## Process of moving operators to csi-operator monorepo

We have come with following flow for moving operators from their own repository into csi-operator mono repo.

### Avoid .gitignore related footguns

Remove any .gitignore entries in the source repository that would match a directory / file that we need. For example, `azure-disk-csi-driver-operator` in .gitignore matched `cmd/azure-disk-csi-driver-operator` directory that we really need not to be ignored. See https://github.com/openshift/csi-operator/pull/110, where we had to fix after merge to csi-operator.

### Move existing code into csi-operator repository

Using git-subtree move your existing operator code to https://github.com/openshift/csi-operator/tree/master/legacy

```
git subtree add --prefix legacy/azure-disk-csi-driver-operator https://github.com/openshift/azure-disk-csi-driver-operator.git master --squash
git subtree push --prefix legacy/azure-disk-csi-driver-operator https://github.com/openshift/azure-disk-csi-driver-operator.git master
```

### Add Dockerfiles for building images from new location

Place a `Dockerfile.<operator>` and `Dockerfile.<operator>.test` at top of csi-operator tree and make sure that you are able to build an image of the operator from csi-operator repository.
Copy link
Member

Choose a reason for hiding this comment

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

We consolidated the various Dockerfile.<operator>.test files into a single Dockerfile.test, there generally shouldn't be any need to add a new one unless some operator is doing more than FROM src.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.


### Update openshift/release to build image from new location
Make a PR to openshift/release repository to build the operator from csi-operator. For example - https://github.com/openshift/release/pull/46233.

1. Update also `storage-conf-csi-<operator>-commands.sh`, the test manifest will be at a different location.
2. Make sure that rehearse jobs for both older versions of operator and newer versions of operator pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a better way to search for rehearsal job names other than looking at jobs in PRs and history? Maybe we could expand on this step a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think when you do /pj-rehearse it doe list the jobs and then you can choose and run.


### Change ocp-build-data repository to ship image from new location

Make a PR to [ocp-build-data](https://github.com/openshift-eng/ocp-build-data) repository to change location of the image etc - https://github.com/openshift-eng/ocp-build-data/pull/4148

1. Notice the `cachito` line in the PR - we need to build with the vendor from legacy/ directory.

2. Ask ART for a scratch build. Make sure you can install a cluster with that build.

```
oc adm release new \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not applicable to OLM based operators - can we have a note or pointer here on testing the scratch build with OLM?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some wording for this.

--from-release=registry.ci.openshift.org/ocp/release:4.15.0-0.nightly.XYZ \
azure-disk-csi-driver-operator=<the scratch build> \
--to-image=quay.io/jsafrane/scratch:release1 \
--name=4.15.0-0.nightly.jsafrane.1

oc adm release extract --command openshift-install quay.io/jsafrane/scratch:release1
```

### Co-ordinating merges in ocp-build-data and release repository

Both PRs in openshift/release and ocp-build-data must be merged +/- at the same time. There is a robot that syncs some data from ocp-build-data to openshift/release and actually breaks things when these two repos use different source repository to build images.

### Enjoy the build from csi-operator repository

After aforementioned changes, your new operator should be able to be built from csi-operator repo and everything should work.

### Moving operator to new structure in csi-operator

So in previous section we merely copied existing code from operator’s own repository into `csi-operator` repository. We did not change anything.

But once your operator has been changed to conform to new code in csi-operator repo, You need to perform following additional steps:
Copy link

Choose a reason for hiding this comment

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

I think the notion of "to conform to new code" deserves more words/details. I'd highlight the following topics:

  • cmd/<new-driver-name>/main.go to be implemented similar to other cmd/*/main.go
  • pkg/driver/<new-driver-name>/<new-driver-name>.go to be implemented similar to other pkg/driver/*/*
  • assets/overlays/<new-driver-name>/base/ and assets/overlays/<new-driver-name>/patches/ to be populated with such yaml files to let generator generate yamls in assets/overlays/<new-driver-name>/generated/ same (or almost same) as assets from legacy/

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not want to tie this enhancement to code and asset structure. What if that changes? An enhancement is not the right place to document implementation details TBH (at least not in great detail).

Copy link

Choose a reason for hiding this comment

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

OK, agreed, let's keep it vague.


1. Make sure that `Dockerfile.<operator>` at top of the `csi-operator` tree refers to new location of code and not older `legacy/<operator>` location.See example of existing Dockerfiles.
gnufied marked this conversation as resolved.
Show resolved Hide resolved
2. After your changes to `csi-operator` are merged, you should remove the old location from cachito - https://github.com/openshift-eng/ocp-build-data/pull/4219
Copy link
Contributor

Choose a reason for hiding this comment

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

I would explicitly mention that Dockerfile changes + ocp-build-data / cachito PR should be merged +/- at the same time, otherwise builds will start failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


Please note the changes to `ocp-build-data`should be merged almost same time as changes into `csi-operator`'s Dockerfile are merged, otherwise we risk builds from breaking.

### Post migration changes

Once migration is complete, we should perform following post migration steps to ensure that we are not left over with legacy stuff:

1. Mark existing `openshift/<vendor>-<driver>-operator` repository as deprecated.
2. Ensure that we have test manifest available in `test/e2e2` directory.
gnufied marked this conversation as resolved.
Show resolved Hide resolved
3. Make changes into `release` repository so as it longer relies on anything from `legacy` directory. See - https://github.com/openshift/release/pull/49655 for example.
4. Remove code from `vendor/legacy` in `csi-operator` repository.


## Implementation History

Major milestones in the life cycle of a proposal should be tracked in `Implementation
Expand Down Expand Up @@ -548,4 +622,4 @@ Advantages:

## Infrastructure Needed [optional]

N/A (other than the usual CI + QE)
N/A (other than the usual CI + QE)