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

add gardenctl hibernate and gardenctl wakeup to hibernate/wakeup shoot cluster #465

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

neo-liang-sap
Copy link
Contributor

@neo-liang-sap neo-liang-sap commented Nov 25, 2020

What this PR does / why we need it:
add gardenctl hibernate and gardenctl wakeup to hibernate/wakeup shoot cluster
Which issue(s) this PR fixes:
Fixes #450

Special notes for your reviewer:

Release note:

after targeting a shoot, you could run "gardenctl hibernate" / "gardenctl wakeup" to hibernate / wakeup a shoot

@neo-liang-sap neo-liang-sap requested a review from a team as a code owner November 25, 2020 07:35
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Nov 25, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 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 25, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 25, 2020
@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 26, 2020
@neo-liang-sap
Copy link
Contributor Author

if no objection i will merge this PR by EOB today

@petersutter petersutter self-requested a review December 1, 2020 08:42
Copy link
Contributor

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

please do not merge unreviewed PRs.

After scrolling quickly over the PR:

  • Why do you fetch all shoots across all namespaces if you just want to get one?
  • There is at least one typo: descirption (Last operation descirption is
  • Instead of polling the shoot, why don't you watch for changes?
  • I would not compare the Description, this would be a fragile solution newShoot.Status.LastOperation.Description == "Shoot cluster state has been successfully reconciled.". The description may be changed any time by the gardener

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Dec 1, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 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 Dec 2, 2020
@neo-liang-sap
Copy link
Contributor Author

neo-liang-sap commented Dec 2, 2020

Hi @petersutter , thanks for your review and comments.

  • i've modified shoot namespace while doing list of shoots, now it's shootList, err := gardenClientset.CoreV1beta1().Shoots(shoot.GetNamespace()).List(metav1.ListOptions{})

  • i've removed description compare, so the typo is also removed

  • for watch for changes, i'm not understanding very clearly. Do you mean move the logic of getting newShoot outside of for loop? in my test, the newShoot object is immutable object, this object will stay the same without reflecting new status change. I'm not sure whether i have expressed myself clearly, if i put logic of getting newShoot outside of for loop the result would be sth like this
    image
    The progress won't change.

So 'each time pulling newShoot object is the simplest way i can come up with, in my test i see the performance is OK , as gardenctl hibernate/wakeup checks shoot status at 1 min interval, this waiting interval takes most of the running time.

Do you have a better way for getting & comparing the newShoot status? it would be much appreciate if you can propose some code snippet for me to reference, thanks!
-Neo

@petersutter
Copy link
Contributor

Hi @neo-liang-sap ,

Implementing a watch is not required of course, but definitely more efficient and instant

@gardener-robot-ci-2 gardener-robot-ci-2 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 Dec 3, 2020
@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 Dec 3, 2020
@neo-liang-sap
Copy link
Contributor Author

Hi @petersutter , thanks, i've changed code to use get to get shoot.
For watch example you provided i took look at them and it seems would takes much effort to implement similar in hibernate/wakeup function, as this function doesn't have strict performance requirement i wonder is it ok to use current implementation (get the shoot object at 1 min interval and check the status) as a easy solution?
Thanks.
-Neo

@gardener-robot-ci-3 gardener-robot-ci-3 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 Dec 3, 2020
@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 Dec 3, 2020
Comment on lines +382 to +384
shoot.Spec.Hibernation = &gardencorev1beta1.Hibernation{
Enabled: &hibernated,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will overwrite hibernation schedule.

@neo-liang-sap
Copy link
Contributor Author

/close as this feature will not be implemented

@neo-liang-sap
Copy link
Contributor Author

per customer requirement i'm reopening this issue #450 (comment)
will continue when available
/reopen

@gardener-robot gardener-robot added the lifecycle/rotten Nobody worked on this for 12 months (final aging stage) label May 19, 2021
@github-actions
Copy link

github-actions bot commented Jul 6, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Sep 7, 2021
@gardener-robot
Copy link

@neo-liang-sap You need rebase this pull request with latest master branch. Please check.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lifecycle/rotten Nobody worked on this for 12 months (final aging stage) needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review PR: needs rebase size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
7 participants