8000 cli: use state file on init and upgrade by msanft · Pull Request #2395 · edgelesssys/constellation · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cli: use state file on init and upgrade #2395

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
Merged
merged 23 commits into from
Oct 9, 2023
Merged

cli: use state file on init and upgrade #2395

merged 23 commits into from
Oct 9, 2023

Conversation

msanft
Copy link
Contributor
@msanft msanft commented Sep 29, 2023 •
edited
Loading

Context

We are in the transition from the ID-File (constellation-id.json) to the state file (constellation-state.yaml). This PR aims to implement part of this transition by using the state-file instead of the ID-file in the init and upgrade operations of a cluster. This PR does not intend to remove the ID-file completely, which should be done in a follow-up PR aligned with AB#3425.

Proposed change(s)

  • Implement basic read / write / merge handlers for the state file.
  • Use the state file in constellation init, where the ClusterValues output is now being populated (in addition to populating the old ID-file)
  • Use the state file in constellation upgrade apply, where the old, pre-upgrade constellation-id.json is being read. It's output, merged with the output of the upgrade itself is written to the state file.

Additional info

  • E2E Test
  • E2E Test Upgrade
  • AB#3424
  • This PR shows a bug in MiniConstellation, where the cluster is not able to initialize ("connection refused" when talking to the bootstrapper node), which is presumably caused by the bootstrapper not starting.

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@netlify
Copy link
netlify bot commented Sep 29, 2023 •
edited
Loading

✅ Deploy Preview for constellation-docs ready!

Name Link
🔨 Latest commit bd9c2df
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/6523d19062733c0008cf41fc
😎 Deploy Preview https://deploy-preview-2395--constellation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@msanft msanft force-pushed the feat/cli/write-cluster-state-file branch 3 times, most recently from f43be93 to 5fa9468 Compare September 29, 2023 13:20
@msanft msanft marked this pull request as ready for review September 29, 2023 13:23
@msanft msanft added this to the v2.12.0 milestone Sep 29, 2023
@msanft msanft added the feature This introduces new functionality label Sep 29, 2023
@msanft msanft marked this pull request as draft October 2, 2023 05:50
@msanft msanft marked this pull request as ready for review October 2, 2023 05:56
@msanft msanft force-pushed the feat/cli/write-cluster-state-file branch from 8d64f11 to a2afff4 Compare October 2, 2023 05:56
@elchead
Copy link
Contributor
elchead commented Oct 2, 2023 •
edited
Loading

i added the AB link :)

elchead
elchead reviewed Oct 2, 2023
View reviewed changes
daniel-weisse
daniel-weisse reviewed Oct 2, 2023
View reviewed changes
@elchead
Copy link
Contributor
elchead commented Oct 2, 2023 •
edited by azure-boards bot
Loading

Originally, the scope of this ticket was not to use the state in upgrade and apply yet. It seems that now the idFile is only relevant to migrate during an upgrade?
It seems that from the original AB#3425 only:

  • upgrade: remove idFile after writing values to clusterValues

is still open or is there anything else left to do to remove it? If that is the case we could also do it in this ticket I think

elchead
elchead reviewed Oct 2, 2023
View reviewed changes
@elchead
Copy link
Contributor
elchead commented Oct 2, 2023

please also run the miniconstellation test in the end

@msanft
8000 Copy link
Contributor Author
msanft commented Oct 2, 2023 •
edited
Loading

Originally, the scope of this ticket was not to use the state in upgrade and apply yet. It seems that now the idFile is only relevant to migrate during an upgrade? It seems that from the original AB#3425 only:

  • upgrade: remove idFile after writing values to clusterValues

is still open or is there anything else left to do to remove it? If that is the case we could also do it in this ticket I think

Not really imo. At this point (in this PR), we still:

  • Write to idFile on create and init.
  • Read the state from idFile during init.

If we choose to implement the fallback from state-file to id-file on upgrade, as Daniel explained above, in this PR, this would be part of - read state file instead of idFile during init + upgrade. Other than that, I don't really see anything of this PR involved in AB#3425
The original ticket mentions to read from idFile and write to the state-file in upgrade apply, which, to me, is what I did in this PR.

@elchead
Copy link
Contributor
elchead commented Oct 2, 2023 •
edited by azure-boards bot
Loading

Originally, the scope of this ticket was not to use the state in upgrade and apply yet. It seems that now the idFile is only relevant to migrate during an upgrade? It seems that from the original AB#3425 only:

  • upgrade: remove idFile after writing values to clusterValues

is still open or is there anything else left to do to remove it? If that is the case we could also do it in this ticket I think

Not really imo. At this point (in this PR), we still:

  • Write to idFile on create and init.
  • Read the state from idFile during init.

If we choose to implement the fallback from state-file to id-file on upgrade, as Daniel explained above, in this PR, this would be part of - read state file instead of idFile during init + upgrade. Other than that, I don't really see anything of this PR involved in AB#3425 The original ticket mentions to read from idFile and write to the state-file in upgrade apply, which, to me, is what I did in this PR.

I originally meant that the idFile is still used in all function signatures to keep the PRs smaller (in AB#3424 (this PR)), but I see that it was unclear and its fine as is now :)
My point is that since we use the new state in the upgrade now, we could already remove the idFile at this stage.
Then only this is left:

  • don't write idFile during init + create anymore
    which seems very small too.

@msanft
Copy link
Contributor Author
msanft commented Oct 2, 2023

I originally meant that the idFile is still used in all function signatures to keep the PRs smaller (in AB#3424 (this PR)), but I see that it was unclear and its fine as is now :) My point is that since we use the new state in the upgrade now, we could already remove the idFile at this stage. Then only this is left:

  • don't write idFile during init + create anymore
    which seems very small too.

Sorry for the misunderstanding here. My intention was to still keep the file intact for now to not break existing development clusters, while relying on the inputs from the state file where possible. I do really like the point of splitting this up into many smaller PRs though!

I already have a separate branch where I'm working on the idFile-Removal. My suggestion would be to open a separate PR with these changes, based on this PR here. This way, reviewers have an easier time checking the changes. I would edit the PR description to reflect exactly which changes I made within which PR then. What do you think?

daniel-weisse
daniel-weisse reviewed Oct 2, 2023
View reviewed changes
@msanft msanft force-pushed the feat/cli/write-cluster-state-file branch from 49fed04 to fa0a066 Compare October 2, 2023 13:52
elchead
elchead reviewed Oct 4, 2023
View reviewed changes
elchead
elchead reviewed Oct 4, 2023
View reviewed changes
elchead
elchead approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor
@elchead elchead left a comment

Choose a reason for hiding this comment

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

lgtm

msanft and others added 23 commits October 9, 2023 12:08
@msanft
[wip] use state file in CLI
8d86fca 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

tidy

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
use state file in CLI
6a8dd5b 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

take clusterConfig from IDFile for compat

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

various fixes

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

wip

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
add GCP-specific values in Helm loader test
a3c6520 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
remove unnecessary pointer
fe61a79 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
write ClusterValues in one step
18e499f 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
move stub to test file
69f1269 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
remove mention of id-file
547b2e3 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
move output to migrateTerraform
331b0b8 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
unconditional assignments converting from idFile
< 9E81 input type="hidden" name="disable_live_updates" value="false" autocomplete="off" data-targets="batch-deferred-content.inputs" />
0d316ee 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
move require block in go modules file
4e745bf 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
fall back to id file on upgrade
c8fea47 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
tidy
0e33603 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
fix linter check
47c8ba6 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
add notice to remove Terraform state check on manual migration
814aabc 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
add name field
b0e6c63 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

fix name tests

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
return early if no Terraform diff
15ed8c9 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
tidy
7438089 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
return infrastructure state even if no diff exists
e31449d 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
add TODO to remove comment
aeeaceb 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
use state-file in miniconstellation
2a0c669 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft @elchead
cli: remove id-file (#2402)
fadadcb 
* remove id-file from `constellation create`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add file renaming to handler

* rename id-file after upgrade

* use idFile on `constellation init`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `constellation verify`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* linter fixes

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `constellation mini`

* remove id-file from `constellation recover`

* linter fixes

* remove id-file from `constellation terminate`

* fix initSecret type

* fix recover argument precedence

* fix terminate test

* generate

* add TODO to remove id-file removal

* Update cli/internal/cmd/init.go

Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>

* fix verify arg parse logic

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add version test

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from docs

* add file not found log

* use state-file in miniconstellation

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `constellation iam destroy`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `cdbg deploy`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

---------

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>
@msanft
use state-file in CI
727a01a 
Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
@msanft
update orchestration docs
bd9c2df
@msanft msanft force-pushed the feat/cli/write-cluster-state-file branch from 403e183 to bd9c2df Compare October 9, 2023 10:10
@github-actions
Copy link
Contributor
github-actions bot commented Oct 9, 2023

Coverage report

Package Old New Trend
cli/internal/cloudcmd 63.90% 63.90% ↔️
cli/internal/cmd 54.70% 55.00% ↗️
cli/internal/helm 49.50% 49.50% 🚧
cli/internal/state [no test files] 94.70% ↗️
cli/internal/terraform 71.40% 71.30% ↘️
debugd/internal/cdbg/cmd [no test files] [no test files] 🚧
internal/file 88.20% 88.30% ↗️

@msanft msanft merged commit 005e865 into main Oct 9, 2023
@msanft msanft deleted the feat/cli/write-cluster-state-file branch October 9, 2023 11:04
msanft added a commit that referenced this pull request Oct 9, 2023
@msanft @elchead
cli: use state file on init and upgrade (#2395)
f91e892 
* [wip] use state file in CLI

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

tidy

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* use state file in CLI

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

take clusterConfig from IDFile for compat

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

various fixes

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

wip

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add GCP-specific values in Helm loader test

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove unnecessary pointer

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* write ClusterValues in one step

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* move stub to test file

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove mention of id-file

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* move output to `migrateTerraform`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* unconditional assignments converting from idFile

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* move require block in go modules file

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* fall back to id file on upgrade

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* tidy

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* fix linter check

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add notice to remove Terraform state check on manual migration

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add `name` field

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

fix name tests

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* return early if no Terraform diff

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* tidy

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* return infrastructure state even if no diff exists

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add TODO to remove comment

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* use state-file in miniconstellation

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* cli: remove id-file (#2402)

* remove id-file from `constellation create`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add file renaming to handler

* rename id-file after upgrade

* use idFile on `constellation init`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `constellation verify`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* linter fixes

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `constellation mini`

* remove id-file from `constellation recover`

* linter fixes

* remove id-file from `constellation terminate`

* fix initSecret type

* fix recover argument precedence

* fix terminate test

* generate

* add TODO to remove id-file removal

* Update cli/internal/cmd/init.go

Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>

* fix verify arg parse logic

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* add version test

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from docs

* add file not found log

* use state-file in miniconstellation

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `constellation iam destroy`

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* remove id-file from `cdbg deploy`

Signed-off-by: Moritz Sanft <58110325+msanft@users.nore
91FA
ply.github.com>

---------

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>

* use state-file in CI

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>

* update orchestration docs

---------

Signed-off-by: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Co-authored-by: Adrian Stobbe <stobbe.adrian@gmail.com>
@malt3 malt3 removed the feature This introduces new functionality label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malt3 malt3 malt3 approved these changes

@derpsteb derpsteb derpsteb approved these changes

@elchead elchead elchead approved these changes

@thomasten thomasten thomasten approved these changes

@daniel-weisse daniel-weisse daniel-weisse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.12.0
Development

Successfully merging this pull request may close these issues.
6 participants
@msanft @elchead @malt3 @derpsteb @thomasten @daniel-weisse
0