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

vintage-migration-changes #354

Merged
merged 36 commits into from
Oct 3, 2023
Merged

vintage-migration-changes #354

merged 36 commits into from
Oct 3, 2023

Conversation

calvix
Copy link
Contributor

@calvix calvix commented Aug 22, 2023

additional field necessary for migration from the vintage cluster
ref issue: https://github.com/giantswarm/giantswarm/issues/27910

the values are filled like this
https://github.com/giantswarm/capi-migration-cli/blob/main/pkg/migrator/capi_app.go#L79-L148

@giantswarm giantswarm deleted a comment from tityosbot Aug 22, 2023
@tityosbot
Copy link

@calvix: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
upgrade b2d246b link /test upgrade

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

1 similar comment
@tityosbot
Copy link

@calvix: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
upgrade b2d246b link /test upgrade

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@calvix calvix self-assigned this Aug 24, 2023
"type": "integer",
"title": "Kubernetes API bind port",
"description": "Kubernetes API bind port used for kube api pod",
"default": 6443
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vintage listen on 443 and in the migration step we need to also set this to port 443, could be removed later

"description": "Kubernetes API bind port used for kube api pod",
"default": 6443
},
"controlPlaneExtraFiles": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly for adding migration script for etcd

@calvix
Copy link
Contributor Author

calvix commented Sep 27, 2023

the example how the new values are filled can be found here capi_app

@calvix calvix marked this pull request as ready for review September 27, 2023 14:39
@calvix calvix requested a review from a team as a code owner September 27, 2023 14:39
helm/cluster-aws/templates/_control_plane.tpl Outdated Show resolved Hide resolved
helm/cluster-aws/templates/_control_plane.tpl Outdated Show resolved Hide resolved
helm/cluster-aws/templates/_control_plane.tpl Outdated Show resolved Hide resolved
helm/cluster-aws/templates/_helpers.tpl Outdated Show resolved Hide resolved
helm/cluster-aws/templates/_machine_pools.tpl Outdated Show resolved Hide resolved
"title": "command"
}
},
"etcdExtraArgs": {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to support different etcd configurations? I was thinking if we need this value at all. Couldn't we just use whatever flags we need in clusterConfiguration.etcd.local.extraArgs and avoid making it configurable?

Copy link
Contributor Author

@calvix calvix Oct 2, 2023

Choose a reason for hiding this comment

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

no, because this extra arguments are only needed for the migration, so it cannot be set globally for all clusters, I need an option to turn specify them on demand fro the migration

anything under .internal should not be used by customers and anything under migration should be possible to remove after migration

@calvix calvix requested review from fiunchinho, AndiDog and a team October 2, 2023 14:43
@calvix
Copy link
Contributor Author

calvix commented Oct 3, 2023

once i get LGTM i will remove the custom user and merge but until then i need the user for testing purposes

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

(helm/cluster-aws/ci/test-wc-minimal-values.yaml) rendered manifest diff
/spec/kubeadmConfigSpec/initConfiguration/localAPIEndpoint/bindPort  (KubeadmControlPlane/org-giantswarm/test-wc-minimal)
  ± value change
    - 0
    + 6443

/spec/kubeadmConfigSpec/joinConfiguration  (KubeadmControlPlane/org-giantswarm/test-wc-minimal)
  + one map entry added:
    controlPlane:
      localAPIEndpoint:
        bindPort: 6443

@calvix calvix merged commit ef51ef6 into master Oct 3, 2023
11 of 12 checks passed
@calvix calvix deleted the vintage-migration-changes branch October 3, 2023 09:08
fiunchinho added a commit that referenced this pull request Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants