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.

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

Pause the deployment config before deleting #9893

Merged
merged 1 commit into from
Jul 22, 2016
Merged

Pause the deployment config before deleting #9893

merged 1 commit into from
Jul 22, 2016

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Jul 18, 2016 •
edited
Loading

Fixes: #9532

0xmichalis
0xmichalis reviewed Jul 18, 2016
View reviewed changes
pkg/deploy/reaper/reaper.go

type updateDeploymentConfigFunc func(d *deployapi.DeploymentConfig)

func (reaper *DeploymentConfigReaper) updateDeploymentWithRetries(namespace, name string, applyUpdate updateDeploymentFunc) (deployment *deployapi.DeploymentConfig, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the named returns

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch from 8475a9c to 087d85a Compare July 18, 2016 12:16
@mfojtik
Copy link
Contributor Author

mfojtik commented Jul 18, 2016

@Kargakis thanks! all fixed.

@0xmichalis
Copy link
Contributor

[test]

@0xmichalis
Copy link
Contributor

Actually can you also update the extended test for the reaper to run in the conformance suite (if it's not already updated) ?

@mfojtik
Copy link
Contributor Author

mfojtik commented Jul 18, 2016

@Kargakis will do

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch 3 times, most recently from 8d8d381 to 14cc49a Compare July 18, 2016 14:37
0xmichalis
0xmichalis reviewed Jul 18, 2016
View reviewed changes
pkg/deploy/reaper/reaper.go
@@ -39,8 +66,8 @@ func (reaper *DeploymentConfigReaper) Stop(namespace, name string, timeout time.
// effect which can cause the deployment to be re-triggered upon the
// config's deletion. See https://github.com/openshift/origin/issues/2721
// for more details.
err := reaper.oc.DeploymentConfigs(namespace).Delete(name)
configNotFound := kerrors.IsNotFound(err)
err = reaper.oc.DeploymentConfigs(namespace).Delete(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

The deletion of the dc needs to move in the end

@0xmichalis
Copy link
Contributor

@Kargakis will do

origin/test/extended/deployments/deployments.go

Line 483 in d26c9ce

g.It("should delete all failed deployer pods and hook pods", func() {

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch 3 times, most recently from 3dedeef to 53cb77d Compare July 19, 2016 08:31
mfojtik
mfojtik reviewed Jul 19, 2016
View reviewed changes
pkg/deploy/reaper/reaper.go
@@ -26,26 +29,44 @@ type DeploymentConfigReaper struct {
pollInterval, timeout time.Duration
}

// pause marks the deployment configuration as paused to avoid triggering new
// deployments.
func (reaper *DeploymentConfigReaper) pause(namespace, name string) (*deployapi.DeploymentConfig, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis I would love to have Pause() func in deployment utils ;-)

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch from 53cb77d to 4203345 Compare July 19, 2016 08:39
0xmichalis
0xmichalis reviewed Jul 19, 2016
View reviewed changes
pkg/deploy/reaper/reaper.go
// Apply the update, then attempt to push it to the apiserver.
applyUpdate(config)
config, err = deploymentConfigs.Update(config)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should retry only on update conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis this is from upstream, if we want that I will fix this in upstream as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should probably fix it there too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed (here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the upstream portion: kubernetes/kubernetes#29188

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch 3 times, most recently from 377970e to 4436fef Compare July 20, 2016 09:26
@0xmichalis
Copy link
Contributor

LGTM [merge]

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch from 4436fef to 0f733dd Compare July 20, 2016 09:48
mfojtik
mfojtik reviewed Jul 20, 2016
View reviewed changes
pkg/deploy/reaper/reaper.go
// If we failed to pause the deployment config, it means we are talking to
// old API that does not support pausing. In that case, we delete the
// deployment config to stay backward compatible.
if !isPaused {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis backward compatibility should be saved

Copy link
Contributor

Choose a reason for hiding this comment

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

sup

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch from 0f733dd to 38bef7d Compare July 20, 2016 09:49
0xmichalis
0xmichalis reviewed Jul 20, 2016
View reviewed changes
pkg/deploy/reaper/reaper.go
for _, rc := range deployments {
if err = rcReaper.Stop(rc.Namespace, rc.Name, timeout, gracePeriod); err != nil {
// Better not error out here...
glog.Infof("Cannot delete ReplicationController %s/%s: %v", rc.Namespace, rc.Name, err)
glog.Infof("Cannot delete ReplicationController %s/%s for deployment %s/%s: %v", rc.Namespace, rc.Name, namespace, name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

for deployment config

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch 2 times, most recently from ed5c015 to bd19c29 Compare July 20, 2016 10:40
@mfojtik mfojtik force-pushed the deploy-pause-reaper branch from bd19c29 to f349944 Compare July 20, 2016 10:40
@mfojtik
Copy link
Contributor Author

mfojtik commented Jul 20, 2016

@Kargakis this now need re-review

  1. Added legacy behavior, so when we fail to set the Pause=true, we delete the deployment config at the top
  2. Added tests for legacy behavior, so we know both pausing and legacy works together

@mfojtik mfojtik force-pushed the deploy-pause-reaper branch from f349944 to 51c2eac Compare July 20, 2016 10:43
@mfojtik
Copy link
Contributor Author

mfojtik commented Jul 20, 2016

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 51c2eac

0xmichalis
0xmichalis reviewed Jul 20, 2016
View reviewed changes
pkg/deploy/cmd/reaper.go
err error
)
deploymentConfigs := reaper.oc.DeploymentConfigs(namespace)
resultErr := wait.Poll(10*time.Millisecond, 1*time.Minute, func() (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't poll too fast - make this 500ms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was copied from upstream :)

@0xmichalis
Copy link
Contributor

One comment, LGTM otherwise

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test ABORTED (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6547/)

@0xmichalis
Copy link
Contributor

LGTM [merge]

@mfojtik
Copy link
Contributor Author

mfojtik commented Jul 20, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 21, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6689/) (Image: devenv-rhel7_4642)

@mfojtik
Copy link
Contributor Author

mfojtik commented Jul 21, 2016 •
edited
Loading

[merge]

(RH registry flake)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 51c2eac

@openshift-bot openshift-bot merged commit 9e17245 into openshift:master Jul 22, 2016
@mfojtik mfojtik deleted the deploy-pause-reaper branch September 5, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mfojtik @0xmichalis @openshift-bot