Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

improve ssh command to work with not joined node #138

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

KristianZH
Copy link

@KristianZH KristianZH commented Nov 8, 2019

What this PR does / why we need it:
This PR fixes 2 issues:

  • 1: Ssh for Azure did not work because Start Instance requires zone-id if we want
    automation. The same zone-id is required for aliyun ecs StopInstance
  • 2: Now will be possible to SSH on node, which does not join the cluster.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
@vpnachev @ialidzhikov @DockToFuture

Release note:

The `ssh` command now works with node name

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 8, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@KristianZH KristianZH changed the title Improve ssh improve ssh command to work with not joined node Nov 8, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 8, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 8, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 8, 2019
@KristianZH KristianZH added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Nov 11, 2019
@DockToFuture
Copy link
Contributor

Would be also nice if we could reflect the ip address behind the node name in the gardenctl ssh cmd. In aws it is refelected in the node name but for aliyun, azure and gcp we don't see it in the output. Would be nice to add a separated output column to the cmd.

@vpnachev
Copy link
Contributor

Would be also nice if we could reflect the ip address behind the node name in the gardenctl ssh cmd. In aws it is refelected in the node name but for aliyun, azure and gcp we don't see it in the output. Would be nice to add a separated output column to the cmd.

@DockToFuture this is possible only for machines that have the respective Node objects in the cluster.
Basically, I do not see why the internal IP is required. The flow should be as following:

  1. gardenerctl ssh will work only for shoot clusters and it will take the name of the Machine object as an argument. It must not depend on the respective Node object because it may not exist.
  2. gardenctl shell will work with any k8s cluster that has Node objects - e.g. seed, shoot, garden. It will use the names of the Node objects. Here the internal IP can be printed as some additional info, but I would not support the IP as an argument of the command.

@DockToFuture
Copy link
Contributor

@vpnachev We never talked about the ip as an argument, we just want to print it to ease debugging.

@vpnachev
Copy link
Contributor

Sure, but let it be only for gardenctl shell, not for gardenctl ssh.

pkg/cmd/ssh.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DockToFuture DockToFuture left a comment

Choose a reason for hiding this comment

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

/lgtm

@KristianZH
Copy link
Author

Sure, but let it be only for gardenctl shell, not for gardenctl ssh.

We discussed this suggestion with @DockToFuture and decided to not implement it for now, because there is no good approach for implementation.

@KristianZH KristianZH added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 12, 2019
@DockToFuture DockToFuture merged commit 447b3e0 into gardener-attic:master Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants