cli: use state file on init and upgrade #2395

msanft · 2023-09-29T08:46:22Z

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 · 2023-09-29T08:46:42Z

✅ 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... Use your smartphone camera to open QR code link.

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

elchead · 2023-10-02T07:23:11Z

i added the AB link :)

cli/internal/state/state.go

cli/internal/cmd/init.go

cli/internal/cmd/init_test.go

cli/internal/cmd/upgradeapply.go

cli/internal/state/state.go

go.mod

elchead · 2023-10-02T07:57:23Z

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

cli/internal/cmd/init_test.go

elchead · 2023-10-02T08:12:47Z

please also run the miniconstellation test in the end

msanft · 2023-10-02T08:18:04Z

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 · 2023-10-02T11:49:21Z

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 · 2023-10-02T11:56:09Z

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?

cli/internal/cmd/upgradeapply.go

cli/internal/state/state_test.go

cli/internal/cmd/upgradeapply.go

elchead

lgtm

Signed-off-by: Moritz Sanft <[email protected]> tidy Signed-off-by: Moritz Sanft <[email protected]>

Signed-off-by: Moritz Sanft <[email protected]> take clusterConfig from IDFile for compat Signed-off-by: Moritz Sanft <[email protected]> various fixes Signed-off-by: Moritz Sanft <[email protected]> wip Signed-off-by: Moritz Sanft <[email protected]>

Signed-off-by: Moritz Sanft <[email protected]>

Signed-off-by: Moritz Sanft <[email protected]> fix name tests Signed-off-by: Moritz Sanft <[email protected]>

Signed-off-by: Moritz Sanft <[email protected]>

* remove id-file from `constellation create` Signed-off-by: Moritz Sanft <[email protected]> * add file renaming to handler * rename id-file after upgrade * use idFile on `constellation init` Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from `constellation verify` Signed-off-by: Moritz Sanft <[email protected]> * linter fixes Signed-off-by: Moritz Sanft <[email protected]> * 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 <[email protected]> * fix verify arg parse logic Signed-off-by: Moritz Sanft <[email protected]> * add version test Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from docs * add file not found log * use state-file in miniconstellation Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from `constellation iam destroy` Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from `cdbg deploy` Signed-off-by: Moritz Sanft <[email protected]> --------- Signed-off-by: Moritz Sanft <[email protected]> Co-authored-by: Adrian Stobbe <[email protected]>

Signed-off-by: Moritz Sanft <[email protected]>

github-actions · 2023-10-09T10:17:13Z

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%	↗️

* [wip] use state file in CLI Signed-off-by: Moritz Sanft <[email protected]> tidy Signed-off-by: Moritz Sanft <[email protected]> * use state file in CLI Signed-off-by: Moritz Sanft <[email protected]> take clusterConfig from IDFile for compat Signed-off-by: Moritz Sanft <[email protected]> various fixes Signed-off-by: Moritz Sanft <[email protected]> wip Signed-off-by: Moritz Sanft <[email protected]> * add GCP-specific values in Helm loader test Signed-off-by: Moritz Sanft <[email protected]> * remove unnecessary pointer Signed-off-by: Moritz Sanft <[email protected]> * write ClusterValues in one step Signed-off-by: Moritz Sanft <[email protected]> * move stub to test file Signed-off-by: Moritz Sanft <[email protected]> * remove mention of id-file Signed-off-by: Moritz Sanft <[email protected]> * move output to `migrateTerraform` Signed-off-by: Moritz Sanft <[email protected]> * unconditional assignments converting from idFile Signed-off-by: Moritz Sanft <[email protected]> * move require block in go modules file Signed-off-by: Moritz Sanft <[email protected]> * fall back to id file on upgrade Signed-off-by: Moritz Sanft <[email protected]> * tidy Signed-off-by: Moritz Sanft <[email protected]> * fix linter check Signed-off-by: Moritz Sanft <[email protected]> * add notice to remove Terraform state check on manual migration Signed-off-by: Moritz Sanft <[email protected]> * add `name` field Signed-off-by: Moritz Sanft <[email protected]> fix name tests Signed-off-by: Moritz Sanft <[email protected]> * return early if no Terraform diff Signed-off-by: Moritz Sanft <[email protected]> * tidy Signed-off-by: Moritz Sanft <[email protected]> * return infrastructure state even if no diff exists Signed-off-by: Moritz Sanft <[email protected]> * add TODO to remove comment Signed-off-by: Moritz Sanft <[email protected]> * use state-file in miniconstellation Signed-off-by: Moritz Sanft <[email protected]> * cli: remove id-file (#2402) * remove id-file from `constellation create` Signed-off-by: Moritz Sanft <[email protected]> * add file renaming to handler * rename id-file after upgrade * use idFile on `constellation init` Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from `constellation verify` Signed-off-by: Moritz Sanft <[email protected]> * linter fixes Signed-off-by: Moritz Sanft <[email protected]> * 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 <[email protected]> * fix verify arg parse logic Signed-off-by: Moritz Sanft <[email protected]> * add version test Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from docs * add file not found log * use state-file in miniconstellation Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from `constellation iam destroy` Signed-off-by: Moritz Sanft <[email protected]> * remove id-file from `cdbg deploy` Signed-off-by: Moritz Sanft <[email protected]> --------- Signed-off-by: Moritz Sanft <[email protected]> Co-authored-by: Adrian Stobbe <[email protected]> * use state-file in CI Signed-off-by: Moritz Sanft <[email protected]> * update orchestration docs --------- Signed-off-by: Moritz Sanft <[email protected]> Co-authored-by: Adrian Stobbe <[email protected]>

msanft requested a review from daniel-weisse September 29, 2023 08:50

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 marked this pull request as ready for review September 29, 2023 13:23

msanft requested review from elchead, derpsteb and malt3 as code owners September 29, 2023 13:23

msanft added this to the v2.12.0 milestone Sep 29, 2023

msanft added the feature This introduces new functionality label Sep 29, 2023

msanft marked this pull request as draft October 2, 2023 05:50

msanft marked this pull request as ready for review October 2, 2023 05:56

msanft force-pushed the feat/cli/write-cluster-state-file branch from 8d64f11 to a2afff4 Compare October 2, 2023 05:56

elchead reviewed Oct 2, 2023

View reviewed changes

cli/internal/state/state.go Show resolved Hide resolved

daniel-weisse reviewed Oct 2, 2023

View reviewed changes

elchead reviewed Oct 2, 2023

View reviewed changes

cli/internal/cmd/init_test.go Show resolved Hide resolved

msanft requested review from daniel-weisse and elchead October 2, 2023 09:49

daniel-weisse reviewed Oct 2, 2023

View reviewed changes

cli/internal/cmd/upgradeapply.go Outdated Show resolved Hide resolved

cli/internal/cmd/upgradeapply.go Outdated Show resolved Hide resolved

msanft force-pushed the feat/cli/write-cluster-state-file branch from 49fed04 to fa0a066 Compare October 2, 2023 13:52

elchead reviewed Oct 4, 2023

View reviewed changes

cli/internal/state/state_test.go Show resolved Hide resolved

elchead reviewed Oct 4, 2023

View reviewed changes

cli/internal/cmd/upgradeapply.go Show resolved Hide resolved

elchead approved these changes Oct 4, 2023

View reviewed changes

malt3 approved these changes Oct 9, 2023

View reviewed changes

msanft and others added 23 commits October 9, 2023 12:08

[wip] use state file in CLI

8d86fca

Signed-off-by: Moritz Sanft <[email protected]> tidy Signed-off-by: Moritz Sanft <[email protected]>

use state file in CLI

6a8dd5b

Signed-off-by: Moritz Sanft <[email protected]> take clusterConfig from IDFile for compat Signed-off-by: Moritz Sanft <[email protected]> various fixes Signed-off-by: Moritz Sanft <[email protected]> wip Signed-off-by: Moritz Sanft <[email protected]>

add GCP-specific values in Helm loader test

a3c6520

Signed-off-by: Moritz Sanft <[email protected]>

remove unnecessary pointer

fe61a79

Signed-off-by: Moritz Sanft <[email protected]>

write ClusterValues in one step

18e499f

Signed-off-by: Moritz Sanft <[email protected]>

move stub to test file

69f1269

Signed-off-by: Moritz Sanft <[email protected]>

remove mention of id-file

547b2e3

Signed-off-by: Moritz Sanft <[email protected]>

move output to migrateTerraform

331b0b8

Signed-off-by: Moritz Sanft <[email protected]>

unconditional assignments converting from idFile

0d316ee

Signed-off-by: Moritz Sanft <[email protected]>

move require block in go modules file

4e745bf

Signed-off-by: Moritz Sanft <[email protected]>

fall back to id file on upgrade

c8fea47

Signed-off-by: Moritz Sanft <[email protected]>

tidy

0e33603

Signed-off-by: Moritz Sanft <[email protected]>

fix linter check

47c8ba6

Signed-off-by: Moritz Sanft <[email protected]>

add notice to remove Terraform state check on manual migration

814aabc

Signed-off-by: Moritz Sanft <[email protected]>

add name field

b0e6c63

Signed-off-by: Moritz Sanft <[email protected]> fix name tests Signed-off-by: Moritz Sanft <[email protected]>

return early if no Terraform diff

15ed8c9

Signed-off-by: Moritz Sanft <[email protected]>

tidy

7438089

Signed-off-by: Moritz Sanft <[email protected]>

return infrastructure state even if no diff exists

e31449d

Signed-off-by: Moritz Sanft <[email protected]>

add TODO to remove comment

aeeaceb

Signed-off-by: Moritz Sanft <[email protected]>

use state-file in miniconstellation

2a0c669

Signed-off-by: Moritz Sanft <[email protected]>

use state-file in CI

727a01a

Signed-off-by: Moritz Sanft <[email protected]>

update orchestration docs

bd9c2df

msanft force-pushed the feat/cli/write-cluster-state-file branch from 403e183 to bd9c2df Compare October 9, 2023 10:10

msanft merged commit 005e865 into main Oct 9, 2023
10 checks passed

msanft deleted the feat/cli/write-cluster-state-file branch October 9, 2023 11:04

malt3 removed the feature This introduces new functionality label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: use state file on init and upgrade #2395

cli: use state file on init and upgrade #2395

msanft commented Sep 29, 2023 •

edited

Loading

netlify bot commented Sep 29, 2023 •

edited

Loading

elchead commented Oct 2, 2023 •

edited

Loading

elchead commented Oct 2, 2023 •

edited by azure-boards bot

Loading

elchead commented Oct 2, 2023

msanft commented Oct 2, 2023 •

edited

Loading

elchead commented Oct 2, 2023 •

edited by azure-boards bot

Loading

msanft commented Oct 2, 2023

elchead left a comment

github-actions bot commented Oct 9, 2023

cli: use state file on init and upgrade #2395

cli: use state file on init and upgrade #2395

Conversation

msanft commented Sep 29, 2023 • edited Loading

Context

Proposed change(s)

Additional info

Checklist

netlify bot commented Sep 29, 2023 • edited Loading

✅ Deploy Preview for constellation-docs ready!

elchead commented Oct 2, 2023 • edited Loading

elchead commented Oct 2, 2023 • edited by azure-boards bot Loading

elchead commented Oct 2, 2023

msanft commented Oct 2, 2023 • edited Loading

elchead commented Oct 2, 2023 • edited by azure-boards bot Loading

msanft commented Oct 2, 2023

elchead left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 9, 2023

Coverage report

msanft commented Sep 29, 2023 •

edited

Loading

netlify bot commented Sep 29, 2023 •

edited

Loading

elchead commented Oct 2, 2023 •

edited

Loading

elchead commented Oct 2, 2023 •

edited by azure-boards bot

Loading

msanft commented Oct 2, 2023 •

edited

Loading

elchead commented Oct 2, 2023 •

edited by azure-boards bot

Loading