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

operator: Move resources cleanup from install to upgrade #8011

Merged
merged 4 commits into from Mar 4, 2020

Conversation

lgarciaaco
Copy link
Contributor

When an upgrade is kicked there are resources that needs to be deleted otherwise the operator continues updating them. This should be a step in the upgrade package rather than a hack in the install action. We have now another step (cleanup) doing this task

Don't perform scale.up step, it makes no sense since the PostUpgradeRun phase will scale up the DeploymentConfigs.

Remove todo DeploymentConfig and BuildConfig as part of the cleanup step in upgrade

Remove invalid trigger from todo BuildConfig

Fixes https://issues.redhat.com/browse/ENTESB-13111

Luis Garcia Acosta added 3 commits March 3, 2020 14:14
When an upgrade is kicked there are resources that needs to be deleted otherwise the operator continues updating them. This should be a step in the upgrade package rather than a hack in the install action. We have now another step (cleanup) doing this task

Dont perform scale.up step, it makes no sense since the PostUpgradeRun phase will scale up the DeploymentConfigs.

Remove todo DeploymentConfig and BuildConfig as part of the cleanup step in upgrade

Remove invalid trigger from todo BuildConfig
Now that we need the config file to install the syndesis app, we also need a way to change it using flags
@pull-request-size pull-request-size bot added the size/l Large label Mar 3, 2020
}
}

time.Sleep(5 * time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there is a race condition with deleting the resources and then the resources being created in the install action. It might be gone by now, or it might be only in my environment. I can remove it and see if it works.

@zregvart
Copy link
Member

zregvart commented Mar 3, 2020

I have difficulty understanding why ce5d6f9 is included in this?

Also can you create the pull request against the master first and then backport to 1.9.x?

@lgarciaaco
Copy link
Contributor Author

I have difficulty understanding why ce5d6f9 is included in this?

@zregvart 73d3a81 introduced the use of configuration pkg in the before action of all install commands, including install app. By using configuration there the operator has to load the syndesis configuration file which by default is /etc/conf/config.yaml. When I am working and running the operator from my editor, I don't have the file there since that file is only in the built operator binary, therefore I need an option to change this to wherever I have my config file.

This happened today to me for install app but I guess other commands are also affected

Also can you create the pull request against the master first and then backport to 1.9.x?

When fixing bugs I like to do the changes in the branch I am fixing. Call me paranoid but master and 1.9.x has diverged in such a way, that there is no guarantee that all the conditions needed to reproduce the bug reported for 1.9.x, are going to be in master.

So if there is no strong argument for stop doing it this way, I would like to keep my PR and manually backport it to master

@zregvart
Copy link
Member

zregvart commented Mar 3, 2020

@lgarciaaco the issue you mention for 73d3a81 should be fixed with #8013 (#8009) and could use with your review.

Based on git diff master..1.9.x install/operator/ the master and the 1.9.x have the following differences:

So you can see that issues like #7983 lead having it solved on 1.9.x and not on master. There is a finite attention span one can afford for this and having to redo the fixes in the future; and this is the least efficient way of making them.

The development happens on master and to have your code integrated with those changes it is best to place it on master first and backport it to 1.9.x.

lgarciaaco pushed a commit to lgarciaaco/syndesis that referenced this pull request Mar 4, 2020
lgarciaaco pushed a commit to lgarciaaco/syndesis that referenced this pull request Mar 4, 2020
@lgarciaaco
Copy link
Contributor Author

@zregvart since you are working on what ce5d6f9 is trying to fix, I am removing it from this PR.

I still don't see a strong argument on why I should do my bugfixes for 1.9.x in master rather than in 1.9.x, but I am up for further offline discussion.

@zregvart
Copy link
Member

zregvart commented Mar 4, 2020

I still don't see a strong argument on why I should do my bugfixes for 1.9.x in master rather than in 1.9.x, but I am up for further offline discussion.

@heiko-braun can I ask you that you have this conversation with @lgarciaaco, I don't think I have anything else to add other than what I said in the comment.

@lgarciaaco lgarciaaco merged commit 2f77369 into syndesisio:1.9.x Mar 4, 2020
lgarciaaco pushed a commit that referenced this pull request Mar 4, 2020
@lgarciaaco
Copy link
Contributor Author

After talking to @zregvart it is clear that changes should be done in master first. I am wrong he is right :)

lgarciaaco pushed a commit that referenced this pull request Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/l Large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants