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

test: Add createcluster action to provisioner cli #1561

Conversation

mkulke
Copy link
Contributor

@mkulke mkulke commented Nov 7, 2023

The test itself will perform an installation of the operator/CAA on the cluster as a setup step, so installing it twice is redundant.

The installation should be idempotent, but having 2 places performing the installation is probably suboptimal and might lead to undesired effects if different provisioner config files are used. If we want to test whether a re-installation works, we should have a dedicated test for that.

I would opt for removing the CAA installation from the provision action, but a createcluster action might be a good middle ground.

The e2e tests will install the operater/CAA itself

Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>
@mkulke mkulke force-pushed the mkulke/add-createcluster-action-to-provsioner-cli branch from ca7aacc to 559c913 Compare November 7, 2023 12:43
@stevenhorsman
Copy link
Member

@katexochen might be interested in this in relation to #1543

@mkulke
Copy link
Contributor Author

mkulke commented Nov 7, 2023

@katexochen might be interested in this in relation to #1543

If we replace cluster provisioning w/ tf code, this PR wouldn't be needed. Not sure how much of an effort the tf conversion will be, maybe that could be a temporary fix?

@stevenhorsman
Copy link
Member

If we replace cluster provisioning w/ tf code, this PR wouldn't be needed. Not sure how much of an effort the tf conversion will be, maybe that could be a temporary fix?

Yes - sorry, I didn't mean to dismiss as not needed, but it might not be strategic long term. I think it's fine as a fix though.
I've though about trying to scrap and re-architecture the actions to be more modular where they validate all their own required variables rather than the fix of actions and environment variables we have at the moment and we could then string together actions easier with different steps like:

  • Create cluster (which could be replaced with terraform)
  • Install CoCo operator including peer-pods
  • Run e2e tests
  • Uninstall CoCo operator including peer-pods
  • Delete cluster (again could terraform)

might I've not got around to prototyping, or articulating it clearly yet, but I think this aligns with this PR

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mkulke
Copy link
Contributor Author

mkulke commented Nov 7, 2023

  • Create cluster (which could be replaced with terraform)
  • Install CoCo operator including peer-pods
  • Run e2e tests
  • Uninstall CoCo operator including peer-pods
  • Delete cluster (again could terraform)

Maybe there's value in keeping steps 2-3 in the test suite, so the tests would always be coupled to a known configuration of a peer-pod setup. step 4 we probably only need to provide in the provisioner cli in tests. removing clusters should be sufficient.

we might also need a step 6 which is garbage collecting resource leftovers, bugs in CAA will leak resources that aren't cleaned up properly. we cannot avoid having those bugs, so we need to account for that in the tests.

@stevenhorsman
Copy link
Member

Maybe there's value in keeping steps 2-3 in the test suite, so the tests would always be coupled to a known configuration of a peer-pod setup. step 4 we probably only need to provide in the provisioner cli in tests. removing clusters should be sufficient.

we might also need a step 6 which is garbage collecting resource leftovers, bugs in CAA will leak resources that aren't cleaned up properly. we cannot avoid having those bugs, so we need to account for that in the tests.

Yeah, those are great points. My thoughts around this were to have the separate actions/units available and independently runnable, but that the test suite would be set to run 2 and 3, but things like the 1 and 5 with are controlled by environment variables would be decoupled. Anyway thoughts for the future!

@mkulke mkulke added the e2e-test label Nov 8, 2023
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

I have not tested this myself, but looks good at first impression. Thanks @mkulke

@mkulke mkulke merged commit d3fc4a6 into confidential-containers:main Nov 16, 2023
24 checks passed
@mkulke mkulke deleted the mkulke/add-createcluster-action-to-provsioner-cli branch November 16, 2023 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants