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

OCPBUGS-17791: CatalogSource: update GoDoc #291

Conversation

stevekuznetsov
Copy link
Member

We won't be setting a memory limit, so udpate the doc.

Impl: operator-framework/operator-lifecycle-manager#3015

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #291 (0f058f5) into master (775e6c6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #291   +/-   ##
=======================================
  Coverage   42.11%   42.11%           
=======================================
  Files          39       39           
  Lines        3495     3495           
=======================================
  Hits         1472     1472           
  Misses       1877     1877           
  Partials      146      146           
Files Changed Coverage Δ
crds/zz_defs.go 35.84% <ø> (ø)
pkg/operators/v1alpha1/catalogsource_types.go 57.69% <ø> (ø)

We won't be setting a memory limit, so udpate the doc.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

Lgtm

@awgreene
Copy link
Member

awgreene commented Aug 21, 2023

/hold
Before we proceed with merging this PR, I want to have a conversation on the backwards incompatible change it introduces. From the related OLM PR, it seems like we're introducing this API change to meet an OpenShift core payload requirement. While some catalogSources are defaults on cluster, users can and do create their own catalogSources.

Are we certain that these API changes won't impact existing user workflows on custom catalogSources? IIRC:

  • a pod that exceeds it's memory limit will be killed.
  • Memory granted to pods isn't yielded back even if the pod doesn't actively need the memory (For example, if the pod needs a bunch of memory at startup it won't abandon it later even if it requires less resources).

So if a user had been using this field, the pod would crash loop assuming that it always requires the same memory that initially caused the crash. If this is the case, I don't see much value in the existing behavior and would be comfortable merging this.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2023
@stevekuznetsov
Copy link
Member Author

So if a user had been using this field

The field has never been published, so it's not possible for anyone to have been using it.

Are we certain that these API changes won't impact existing user workflows on custom catalogSources?

Yes, as you note the only change here is that a pod that was crash-looping would no longer be.

@awgreene
Copy link
Member

Thanks @stevekuznetsov
/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2023
@awgreene
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@openshift-ci
Copy link

openshift-ci bot commented Aug 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2023
@openshift-merge-robot openshift-merge-robot merged commit 44b1863 into operator-framework:master Aug 21, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants