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

K8sInstallerConfig controller implementation added #526

Merged

Conversation

mayur-tolexo
Copy link
Contributor

@mayur-tolexo mayur-tolexo commented May 6, 2022

What this PR does / why we need it:
K8sInstallerConfig controller will be generation the installation secret
for the bundleType to install on byohost

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #517

Additional information for reviewer
Normal Reconcile flow

  • If the resource does not have a ByoMachine owner, exit the reconciliation
  • If the Cluster to which this resource belongs cannot be found, exit the reconciliation
  • If ByoMachine.status.condition. ByoHostReady reason is not equal to InstallationSecretNotAvailableReason, exit the reconciliation
  • If status.ready is true, exit the reconcilaiton
  • The name for the installation secret will be same as of K8sInstallerConfig
  • Try to retrieve the Secret with the name from the previous step
    • If it does not exist, generate installation/uninstallation data using ByoMachine.status.hostinfo details and create the Secret with following data:
      • install (string): contains installation bash script
      • uninstall (string): contains uninstallation bash script
    • Variables: need to keep these variables in script to parse by the Byo agent.
      • {{.BundleDownloadPath}}: path on host where bundle will be downloaded by byoh agent
  • Set status.installationSecret to the generated secret object reference
  • Set status.ready = true
  • Patch the resource to persist changes

Delete Reconcile flow

  • If the resource does not have a ByoMachine owner, exit the reconciliation
  • If the Cluster to which this resource belongs cannot be found, exit the reconciliation
  • Fetch the secret from the status.installationSecret reference
    • If error occurred except not found then return the error
    • Else delete the secret
  • Remove finalizer from K8sInstallerConfig resource
  • Patch the resource to persist changes

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #526 (0b2a49d) into main (ca6f3e9) will increase coverage by 1.55%.
The diff coverage is 80.26%.

❗ Current head 0b2a49d differs from pull request most recent head 0cfbfc5. Consider uploading reports for the commit 0cfbfc5 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
+ Coverage   63.67%   65.22%   +1.55%     
==========================================
  Files          26       27       +1     
  Lines        1938     2151     +213     
==========================================
+ Hits         1234     1403     +169     
- Misses        625      657      +32     
- Partials       79       91      +12     
Impacted Files Coverage Δ
...rs/infrastructure/k8sinstallerconfig_controller.go 78.64% <78.64%> (ø)
agent/installer/bundle_downloader.go 71.87% <100.00%> (+2.21%) ⬆️
agent/installer/installer.go 78.10% <100.00%> (ø)

@mayur-tolexo mayur-tolexo force-pushed the installer-controller branch 13 times, most recently from eea1a08 to 470dbfe Compare May 10, 2022 06:22
@mayur-tolexo mayur-tolexo marked this pull request as ready for review May 10, 2022 06:23
common/installer/downloader.go Outdated Show resolved Hide resolved
common/installer/internal/algo/ubuntu20_4k8s.go Outdated Show resolved Hide resolved
common/utils.go Outdated Show resolved Hide resolved
@dharmjit
Copy link
Contributor

@mayur-tolexo, This PR is missing tests for K8sInstallerConfigReconciler

@mayur-tolexo
Copy link
Contributor Author

mayur-tolexo commented May 11, 2022

@mayur-tolexo, This PR is missing tests for K8sInstallerConfigReconciler

@dharmjit , Sorry, I didn't get it. Which test case is missing?

@mayur-tolexo mayur-tolexo force-pushed the installer-controller branch 2 times, most recently from d199911 to bbb92f2 Compare May 11, 2022 08:35
@dharmjit
Copy link
Contributor

@mayur-tolexo, This PR is missing tests for K8sInstallerConfigReconciler

@dharmjit , Sorry, I didn't get it. Which test case is missing?

Oh my bad, somehow I missed - k8sinstallerconfig_controller_test.go

@mayur-tolexo mayur-tolexo force-pushed the installer-controller branch 3 times, most recently from 0cfbfc5 to 7c1631f Compare May 12, 2022 12:03
Copy link
Contributor

@dharmjit dharmjit left a comment

Choose a reason for hiding this comment

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

LGTM

dharmjit
dharmjit previously approved these changes May 12, 2022
K8sInstallerConfig controller will be generation the installation secret
for the bundleType to install on byohost

test cases added

To increase the test coverage, few test cases added for
k8sinstallercontroller
- should not return reconcile request if ByoMachine InstallerRef doesn't exists
- should not return reconcile request if ByoMachine InstallerRef doesn't refer to K8sInstallerConfitTemplate
- should return reconcile request if ByoMachine refer to K8sInstallerConfigTemplate installer

Signed-off-by: Mayur Das <[email protected]>
Copy link
Contributor

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

LGTM

@shubham14bajpai shubham14bajpai merged commit 779caac into vmware-tanzu:main May 13, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement K8sInstallerConfig controller
5 participants