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

ROX-11407: Support for Configurable Resource Limits for Central & Scanner #243

Merged
merged 12 commits into from
Aug 9, 2022

Conversation

mtesseract
Copy link
Contributor

Description

  • Extends the API for modelling resource settings.
  • Introduces new admin endpoint for creating centrals with non-default resource settings.

There are plenty of generated files, which should, of course, be filtered out while reviewing.
Also, my editor has (unfortunately) streamlined the used quotation marks ("..." vs '...') in the API YAMLs, hence the diff to these files is unnecessary large.

Implementing the Update call for modifying resource limits will be done in a separate PR.

Test has been added.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Documentation added if necessary
  • CI and all relevant tests are passing

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup OCM_OFFLINE_TOKEN=<ocm-offline-token> OCM_ENV=development
make verify lint binary test test/integration

@mtesseract mtesseract requested a review from kovayur July 29, 2022 08:10
@mtesseract mtesseract changed the title Mc/configurable central resource limits ROX-11407: Support for Configurable Resource Limits for Central & Scanner Jul 29, 2022
@mtesseract
Copy link
Contributor Author

/retest

@@ -22,7 +22,7 @@ tags:

paths:
# Endpoints for data plane communications
'/api/rhacs/v1/agent-clusters/{id}/status':
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we need a pre-commit hook for yaml formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea

Copy link
Contributor

@kovayur kovayur left a comment

Choose a reason for hiding this comment

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

LGTM, Good job @mtesseract

@openshift-ci openshift-ci bot added the lgtm label Aug 3, 2022
@mtesseract mtesseract force-pushed the mc/configurable-central-resource-limits branch from 5605f39 to f7f3a64 Compare August 4, 2022 12:47
@openshift-ci openshift-ci bot removed the lgtm label Aug 4, 2022
@mtesseract mtesseract force-pushed the mc/configurable-central-resource-limits branch 2 times, most recently from 95f0cee to 6ea0646 Compare August 5, 2022 10:03
@mtesseract
Copy link
Contributor Author

@kovayur, I had to rework the PR due to some Issues I have found. Let me explain.

  • I did the following change:
-	Central     api.JSON `json:"central"` // Schema is defined by #/components/schemas/CentralSpec from the public API.
-	Scanner     api.JSON `json:"scanner"` // Schema is defined by #/components/schemas/ScannerSpec from the public API.
+	Central     api.JSON `json:"central"` // Schema is defined by dbapi.CentralSpec
+	Scanner     api.JSON `json:"scanner"` // Schema is defined by dbapi.ScannerSpec

which means that the internal representation of the central and scanner spec is defined by dbapi.CentralSpec and dbapi.ScannerSpecinstead of the OpenAPI generated models. I find this more logical (API models only for conversion at service boundaries) and also more reliable, because the models indbapi` are more strongly typed.

  • I have streamlined the conversion functions in resourceutil.go and the validation functions in internal/dinosaur/pkg/handlers/dinosaur.go(there was also a bug in there).

  • internal/dinosaur/pkg/presenters/dinosaur.go: Only attempt to unmarshal central and scanner in case they are non-empty. Same in internal/dinosaur/pkg/presenters/managedcentral.go.

  • Fix a rebase error in internal/dinosaur/pkg/presenters/managedcentral.go.

  • Fix a bug in route_loader.go which caused the wrong dinosaur handler to react to admin calls.

  • Then finally: The internal refactoring of the types triggered a rather mysterious issue in internal/dinosaur/pkg/config/dataplane_cluster_config.go: due to a transitive (non-obvious) dependency on the k8s controller-runtime package we were hit by flag redefined: kubeconfig: allow double vendoring this library but still register flags on behalf of users kubernetes-sigs/controller-runtime#878. This caused fleet-manager to panic during startup. Until this is fixed in upstream I have added an ugly workaround, which is still the best I was able to come up with in the above file. It checks if the flag has already been defined and if so it modifies it to be in the shape that our application code expects. This seems to work nicely.

@mtesseract mtesseract force-pushed the mc/configurable-central-resource-limits branch from 73c4255 to b8f1c06 Compare August 5, 2022 11:27
@openshift-ci openshift-ci bot added the lgtm label Aug 5, 2022
@mtesseract mtesseract requested a review from kovayur August 5, 2022 13:26
@kovayur
Copy link
Contributor

kovayur commented Aug 5, 2022

Thanks for the explanation @mtesseract

I find this more logical (API models only for conversion at service boundaries)
Yes, this is more coherent, I agree

@mtesseract mtesseract force-pushed the mc/configurable-central-resource-limits branch from 03ffa72 to 0ef0ea3 Compare August 8, 2022 07:34
@openshift-ci openshift-ci bot removed the lgtm label Aug 8, 2022
@mtesseract mtesseract force-pushed the mc/configurable-central-resource-limits branch from 0ef0ea3 to 1e0f27c Compare August 8, 2022 08:05
@mtesseract
Copy link
Contributor Author

/retest

@mtesseract mtesseract requested review from kovayur and removed request for johannes94 August 9, 2022 09:21
@openshift-ci openshift-ci bot added the lgtm label Aug 9, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kovayur, mtesseract

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

@mtesseract mtesseract merged commit 4f29bb2 into main Aug 9, 2022
@mtesseract mtesseract deleted the mc/configurable-central-resource-limits branch August 9, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants