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

Option needed for new ems_refresh.openshift.store_unused_images setting #11

Merged
merged 1 commit into from
Aug 29, 2017
Merged

Option needed for new ems_refresh.openshift.store_unused_images setting #11

merged 1 commit into from
Aug 29, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented May 3, 2017 •
edited
Loading

#kubernetes part of ManageIQ/manageiq#14662 after repo split.
This will have to be merged before openshift part ManageIQ/manageiq-providers-openshift#9.

Strawman alternative/baseline for ManageIQ/manageiq#14628.
Will allow to still get_container_images=true, but instead of saving metadata (labels etc) on all images openshift gave us, save it only for images that have been mentioned by pods.

Here called store_new_images because this function doesn't have the context to understand which images are "used".
For store_unused_images=false setting, it'll be called twice:

  • from parse_pods with true, adding all images
  • from parse_openshift_image with false, only enriching previous images

cc @agrare @enoodle @zeari

This was referenced May 3, 2017
Merged
Closed
enoodle
enoodle approved these changes May 3, 2017
View reviewed changes
Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

LGTM

@cben
Copy link
Contributor Author

cben commented May 8, 2017

@simon3z @blomquisg @agrare please review/merge...

agrare
agrare approved these changes May 8, 2017
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@cben
Copy link
Contributor Author

cben commented May 10, 2017

The relevant BZ is probably https://bugzilla.redhat.com/show_bug.cgi?id=1436176 "OPenShift Refresh duration exceeds default two hour timeout and grows > 8GB never fully completing"

@cben
Copy link
Contributor Author

cben commented May 10, 2017

I believe this is ready
@miq-bot assign simon3z

@cben
Copy link
Contributor Author

cben commented May 10, 2017

@miq-bot add-label enhancement

@cben cben force-pushed the config_unused_images branch from 75820e2 to bfde9d4 Compare May 22, 2017 10:11
@cben
Copy link
Contributor Author

cben commented May 22, 2017

ping @simon3z.
rebased both again. this should be merged first, then will rerun ManageIQ/manageiq-providers-openshift#9 tests.

@simon3z
Copy link
Contributor

simon3z commented May 22, 2017

@cben let's focus on the graph refresh for now.

@cben
Copy link
Contributor Author

cben commented May 22, 2017 via email

@simon3z
Copy link
Contributor

simon3z commented May 22, 2017

Sure, but is anything stopping this from being merged?

@cben yes capacity in supporting different flows of collection for images (that both needs to be tested).
On a related note, isn't ManageIQ/manageiq#14808 related to this? In order to avoid losing information about openshift images that were scanned, etc.

@cben
Copy link
Contributor Author

cben commented Jul 9, 2017 •
edited
Loading

@simon3z @enoodle I'm closing these for now, if you'll want to revive them, it should be a simple rebase.

Good point that this would increase exposure to #103 (issue extracted from ManageIQ/manageiq#14808).

@cben cben closed this Jul 9, 2017
@cben
Option needed for new ems_refresh.openshift.store_unused_images setting
004437c 
Allows to still get_container_images=true, but instead of saving
metadata (labels etc) on all images openshift gave us,
save it only for images that have been mentioned by pods.

Here called store_new_images because this function doesn't have the
context to understand which images are "used".
For store_unused_images=false, it'll be called twice:
- from parse_pods with true, adding all images
- from parse_openshift_image with false, only enriching previous images
@cben cben mentioned this pull request Aug 28, 2017
Closed
@cben
Copy link
Contributor Author

cben commented Aug 28, 2017

Reviving, @enoodle @agrare PTAL. (openshift PR WIP, will reopen soon)

@simon3z: yes it will increase image disconnection (#103) but so will get_container_images: false which is already merged. This is more expensive but strictly safer.
We have a user env where this is neccessary (they need image labels in reports but don't need images that are not running).

@agrare
Copy link
Member

agrare commented Aug 28, 2017

@cben 👍 from me

@simon3z
Copy link
Contributor

simon3z commented Aug 29, 2017

@cben ok on the idea but I wish we had a better code solution (I wish we had two different methods to call, one to just enrich and one to add new found images, of course they could reuse some common code).

I don't want to block this if it's urgent but I really hope we can improve it.

@cben cben reopened this Aug 29, 2017
@cben cben force-pushed the config_unused_images branch from bfde9d4 to 004437c Compare August 29, 2017 11:55
@miq-bot
Copy link
Member

miq-bot commented Aug 29, 2017

Checked commit cben@004437c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@cben
Copy link
Contributor Author

cben commented Aug 29, 2017

Ack, I'd love to refactor the image code.
parse_container_image is only called twice — from pods and from openshift images.
I can extract the bulk of it into 2 helpers get_or_store_container_image_registry,
get_or_store_container_image; then I'd call parse_image_name and those 2 directly, with a variation when store_unused_images=false.
But that's starting to resemble InventoryCollection#find_or_build_by primitive a lot, and it's funny to reinvent that wheel in data_index then possibly convert to InventoryCollection.
I also have an upcoming new use for data_index for tag mapping, and want to think if there is a cleaner way.

=> For now this is small and backportable, I'd like to merge it, and do a followup after I think more...

Ladas
Ladas reviewed Aug 29, 2017
View reviewed changes
app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb
@@ -1100,7 +1100,8 @@ def parse_container_state(state_hash)
res
end

def parse_container_image(image, imageID)
# may return nil if store_new_images = false
def parse_container_image(image, imageID, store_new_images: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

store_new_images or store_unused_images ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cben @Ladas also, it's so confusing with the other variables named stored_container_image_*

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 function doesn't have the context to understand what's used.
It gets called twice, first adding images from pods, second maybe not adding images from openshift registry.

Everybody asked this. OK, I see I better rewrite this.

@cben cben changed the title Option needed for new ems_refresh.openshift.store_unused_images setting [WIP] Option needed for new ems_refresh.openshift.store_unused_images setting Aug 29, 2017
@miq-bot miq-bot added the wip label Aug 29, 2017
@cben cben changed the title [WIP] Option needed for new ems_refresh.openshift.store_unused_images setting Option needed for new ems_refresh.openshift.store_unused_images setting Aug 29, 2017
@simon3z simon3z merged commit 0046549 into ManageIQ:master Aug 29, 2017
@agrare agrare added euwe/yes and removed wip labels Aug 29, 2017
@cben
Copy link
Contributor Author

cben commented Aug 29, 2017

reusing BZ https://bugzilla.redhat.com/show_bug.cgi?id=1436176, same as get_container_images and other band-aids?

@agrare
Copy link
Member

agrare commented Aug 29, 2017

@cben that BZ is already in post so I think we should open a new BZ

@agrare
Copy link
Member

agrare commented Aug 29, 2017

BZ https://bugzilla.redhat.com/show_bug.cgi?id=1486483

@moolitayer moolitayer added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 30, 2017
cben pushed a commit to cben/manageiq that referenced this pull request Aug 30, 2017
@simon3z @cben
Backport
162caf9 
ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

https://bugzilla.redhat.com/show_bug.cgi?id=1486483

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on fine,
adjusted for different test method signature :connected vs :archived)
@cben cben mentioned this pull request Aug 30, 2017
Merged
cben pushed a commit to cben/manageiq that referenced this pull request Aug 31, 2017
@simon3z @cben
Backport
2127843 
ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on euwe,
adjusted for different test method signature :connected vs :archived,
simulating missing stub_settings_merge,
adjusted for different VCR cassette)

https://bugzilla.redhat.com/show_bug.cgi?id=1486483
@cben cben mentioned this pull request Aug 31, 2017
Merged
1 task
@simaishi
Copy link

Backported to Euwe via ManageIQ/manageiq#15917

@cben cben mentioned this pull request Sep 6, 2017
Merged
@simaishi
Copy link

Backported to Fine via ManageIQ/manageiq#15907

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@simon3z @d-m-u
Backport
1dc219c 
ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

https://bugzilla.redhat.com/show_bug.cgi?id=1486483

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on fine,
adjusted for different test method signature :connected vs :archived)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas left review comments

@simon3z simon3z simon3z left review comments

@agrare agrare agrare approved these changes

@enoodle enoodle enoodle approved these changes

Assignees

@simon3z simon3z

Labels
enhancement euwe/backported fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cben @simon3z @agrare @miq-bot @simaishi @Ladas @enoodle @moolitayer