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

feat: add pre-flight check for installing ocm-controller #565

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Nov 7, 2023

Description

Add a pre-flight check to see if all conditions are met that are required for the controller to work before installing it.

Closes open-component-model/ocm-controller#285

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

  • Related Issue # (issue)
  • Closes # (issue)
  • Fixes # (issue)

Remove if not applicable

Screenshots

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

  • 📜 README.md
  • 🙅 no documentation needed

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@github-actions github-actions bot added the size/m Medium label Nov 7, 2023
@Skarlso Skarlso requested review from phoban01 and souleb November 7, 2023 08:57
@Skarlso Skarlso force-pushed the fix-ocm-install-command branch 5 times, most recently from c3d4d83 to 6e594c4 Compare November 7, 2023 12:20
@Skarlso Skarlso marked this pull request as draft November 7, 2023 12:26
@Skarlso Skarlso force-pushed the fix-ocm-install-command branch 2 times, most recently from 8d12cf3 to 09671fc Compare November 7, 2023 12:45
@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 7, 2023

./bin/ocm controller install
► running pre-install check
► installing prerequisites
► installing cert-manager with version v1.13.2
✔ successfully fetched install file
► applying to cluster...
► waiting for ocm deployment to be ready
✔ cert-manager successfully installed
► creating certificate for internal registry
✔ successfully installed prerequisites
► installing ocm-controller with version latest
► got latest version "v0.15.0"
✔ successfully fetched install file
► applying to cluster...
► waiting for ocm deployment to be ready
✔ ocm-controller successfully installed

k get pods -A
NAMESPACE            NAME                                             READY   STATUS    RESTARTS   AGE
cert-manager         cert-manager-6954d7bbbf-sqcbf                    1/1     Running   0          46s
cert-manager         cert-manager-cainjector-84bdff4846-fbcgf         1/1     Running   0          46s
cert-manager         cert-manager-webhook-85b6b76d9b-6lmgm            1/1     Running   0          46s
kube-system          coredns-5d78c9869d-t92c4                         1/1     Running   0          8m23s
kube-system          coredns-5d78c9869d-v8p48                         1/1     Running   0          8m23s
kube-system          etcd-ocm-test-control-plane                      1/1     Running   0          8m38s
kube-system          kindnet-kfjjw                                    1/1     Running   0          8m24s
kube-system          kube-apiserver-ocm-test-control-plane            1/1     Running   0          8m38s
kube-system          kube-controller-manager-ocm-test-control-plane   1/1     Running   0          8m38s
kube-system          kube-proxy-zmnph                                 1/1     Running   0          8m24s
kube-system          kube-scheduler-ocm-test-control-plane            1/1     Running   0          8m38s
local-path-storage   local-path-provisioner-6bc4bddd6b-xm5pt          1/1     Running   0          8m23s
ocm-system           ocm-controller-84987cb995-n477k                  1/1     Running   0          23s
ocm-system           registry-8667fbb848-kxnc9                        1/1     Running   0          23s

@Skarlso Skarlso marked this pull request as ready for review November 7, 2023 12:51
@Skarlso Skarlso requested a review from robertwol November 7, 2023 12:52
@Skarlso Skarlso force-pushed the fix-ocm-install-command branch from 09671fc to cb4ca4a Compare November 7, 2023 13:07
Copy link
Contributor

@mandelsoft mandelsoft left a comment

Choose a reason for hiding this comment

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

Just a question, the command accesses an ocm github repository to fetch things.
I thought, the installation should be based on an OCM component version.

Som the user should be able to specify

  • an OCM repository (via the repo spec syntax provided by the lib, like the other commands)
  • (optionall) overwrite the component containing the installation files.

This repository (and the appropriate component) can then be searched for the latest version. and the content can be download from the select component version.

@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 7, 2023

Nope. This one isn't designed to use components. That's the mpas bootstraper. This one is fine fetching things from source and repository of the main thing.

@phoban01
Copy link
Contributor

phoban01 commented Nov 7, 2023

What's the value of introducing a component here other than adding another layer? I'm all for dog-fooding but in this case introducing a component doesn't add any value.

@morri-son
Copy link
Contributor

What's the value of introducing a component here other than adding another layer? I'm all for dog-fooding but in this case introducing a component doesn't add any value.

We would be able to bundle all required components into one OCM component in their correct versions, e.g. specifying the hard-coded version of the cert-manager in https://github.com/open-component-model/ocm/pull/565/files#diff-331ec72911a0b802a9cc3c79d465b30809b711218a47cfc6983e92b3ae0cc81fR75. "we would be able" would also mean we need to create new version of that component with each release. value for the end user: not really different than with the current implementation, but the user needs to know the name of component and its OCM repository. Value for the ocm story: consistency when it comes how to bundle and transport artifacts. Why do we do it differently for mpas? because of the higher number of components?

@Skarlso
Copy link
Contributor Author

Skarlso commented Nov 7, 2023

specifying the hard-coded version of the cert-manager in

FWIW, it's a default version that you can override with a parameter. :)

Why do we do it differently for mpas? because of the higher number of components?

Partially, yes. Also, for MPAS there was a requirement to provide offline access. This is achieved by providing the ability to create offline artifacts which then can be used for transferring to a specific repository. That repository then can be used for bootstrapping.

Here, there was no such requirement. It can just fetch things from the source. :)

@phoban01
Copy link
Contributor

phoban01 commented Nov 7, 2023

We would be able to bundle all required components into one OCM component in their correct versions, e.g. specifying the hard-coded version of the cert-manager

We could do this but it doesn't help or improve the installation process in any way. The artifact in this case is produced by the release pipeline for the controller, which bundles the resources into a specific release artifact.

Value for the ocm story: consistency when it comes how to bundle and transport artifacts.

Generally speaking I totally agree with this point. But in this specific case, we would be wrapping an artifact in a component only to unwrap it elsewhere. I see this as an entirely unnecessary obfuscation.

Why do we do it differently for mpas? because of the higher number of components?

MPAS is different because we want to bundle everything that is needed to bootstrap MPAS and ensure that it can be transported. When boostrapping MPAS we also care a lot about the sequence and don't just dump everything at the cluster. In the case of the ocm-controller it is not so complicated and we can just dump the prerequisites at the cluster from a single artifact.

I think there is a separate discussion to be had about publishing components for all OCM projects and we should certainly do that I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/m Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ocm controller install does not create successful deployments
4 participants