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

Insure that any resource that is prunable can be pruned #326

Merged
merged 5 commits into from
Sep 11, 2018

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Aug 8, 2018

Resources with deploy_method replace* don't go through an apply cycle. Since pruning happens in the apply cycle, they are never pruned. Ensure that everything that is prunable can actually be pruned.

Allow PDBs to be pruned too
#279

@dturn dturn changed the title Replace Apply, for pruning Insure that any resource that is prunable can be pruned Aug 8, 2018
@dturn
Copy link
Contributor Author

dturn commented Aug 8, 2018

It looks like the apply issue has been resolved: kubernetes/kubernetes#53379

Think we should support pruning on replaceable resources?

@KnVerey
Copy link
Contributor

KnVerey commented Aug 9, 2018

If I'm remembering correctly, the :replace method was add because of that bug. I'd say if the fix was cherry-picked to all currently supported versions (looks like it was), we should go back to applying all CRs. At one point we were discussing relying on the deploy clearing the non-subresource status, but since we decided against that, I don't think we have a good reason to be replacing anymore.

Looks like we still need replace_force though, as the PDB issue is still open. I'd consider the fact that those aren't pruned to be a bug, but not relevant to your current project.

@dturn
Copy link
Contributor Author

dturn commented Aug 9, 2018

@Shopify/cloudx 👀 please

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

  • Statefulservice still has replace in it
  • Please make sure to 🎩 the branch before merging by locally deploying a test app with multiple Shopify CRs to a cluster that actually supports them. Make sure to try a deploy that includes a change to the CR's spec (i.e. do a deploy that would have actually triggered the bug we were working around with :replace). There is pretty much zero test coverage for those files because our controllers are not deployed in CI (yet another reason to get rid of them in favour of the dynamic feature!).

@dturn
Copy link
Contributor Author

dturn commented Aug 15, 2018

Statefulservice still has replace in it

This was intentional. I was unsure why SS are replace since doesn't seem like they would be part of the CRD merge bug.

@dturn
Copy link
Contributor Author

dturn commented Aug 24, 2018

🎩 with cloud-portal in the staging cluster by increasing & decreasing redis size. Let me know if you want to other resources.

@KnVerey
Copy link
Contributor

KnVerey commented Sep 5, 2018

This was intentional. I was unsure why SS are replace since doesn't seem like they would be part of the CRD merge bug.

Are you thinking StatefulSet rather than StatefulService, perhaps? StatefulSet are a core resource and are already applied; StatefulService are CRs and were replaced for the same reason as the others. Confusing naming for sure.

with cloud-portal in the staging cluster by increasing & decreasing redis size. Let me know if you want to other resources

I'd suggest testing a change with a more complicated patch just in case. For example. a change inside an array or a hash (I'm actually curious which strategy it uses by default... for core resources it is field-specific). ES probably has the most elaborate schema, and cumulus-cat has one so could be a good target for testing.

@dturn
Copy link
Contributor Author

dturn commented Sep 10, 2018

Are you thinking StatefulSet rather than StatefulService, perhaps? StatefulSet are a core resource and are already applied; StatefulService are CRs and were replaced for the same reason as the others. Confusing naming for sure.

Yep I definitely got StatefulService confused with StatefulSet

I'll 🎩 something more complicated after fixing up StatefulSet

@dturn
Copy link
Contributor Author

dturn commented Sep 10, 2018

🎩 a more complicated change went smoothly.

@dturn
Copy link
Contributor Author

dturn commented Sep 11, 2018

Ready for final 👀

@KnVerey
Copy link
Contributor

KnVerey commented Sep 11, 2018

🚢

@dturn dturn merged commit 4a5b673 into master Sep 11, 2018
@dturn dturn deleted the cloudsql-replace-apply branch September 11, 2018 19:21
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.

None yet

3 participants