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

[WIP] Virtual attributes for Containers and friends (performance changes) #22575

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jun 21, 2023

depends upon:

@agrare Can we outright claim that PersistentVolume#parent is always an Ems?
I get that a ContainerVolume#parent is polymorphic, but curious about the child class (i.e.: PersistentVolume). If not, may need to drop that change.

Numbers

ms query qry ms rows comments
347.2 32 53.2 128 /container_node/report_data#node-index
308.8 11 10.3 28 /container_node/report_data#node-after
11.1% 66% 80.6% 78% diff
ms query qry ms rows comments
428.0 32 123.2 88 /container_group/report_data#group-index
336.5 11 65.5 28 /container_group/report_data#group-after
21.4% 66% 46.8% 68% diff
ms query qry ms rows comments
424.3 51 98.7 48 /container_image/report_data
345.1 11 34.4 28 /container_image/report_data
18.7% 78% 65.1% 42% diff

no longer:

ms query qry ms rows comments
278.6 31 12.7 48 /persistent_volume/report_data#before
258.7 11 9.9 28 /persistent_volume/report_data#after
7.1% 65% 22.0% 42% diff

@agrare
Copy link
Member

agrare commented Jun 21, 2023

@kbrock it definitely defaults to the EMS I don't know if that is the only possible option but it does appear like it based on the kubernetes parser

@kbrock
Copy link
Member Author

kbrock commented Aug 1, 2023

update:

  • rebased master
  • updated test to mark now sql friendly virtual attribute as ... sql friendly.

@kbrock kbrock changed the title Virtual attributes for Containers and friends (performance changes) [WIP] Virtual attributes for Containers and friends (performance changes) Aug 1, 2023
@miq-bot miq-bot added the wip label Aug 1, 2023
@kbrock kbrock force-pushed the container_node_n1_deux branch 2 times, most recently from be59dd4 to 0d00279 Compare August 1, 2023 22:30
@kbrock
Copy link
Member Author

kbrock commented Aug 1, 2023

update:

  • extracted container volume assumption that the persistent volume is always associated with an ems.

The savings are very good, but I am not ready to put my foot down on that one.
The other changes are pretty much a no brainer.

UN-WIP: concerning commit around container volume and ems relationships

update:

  • rubocops: add :dependent, :inverse_of

@kbrock kbrock changed the title [WIP] Virtual attributes for Containers and friends (performance changes) Virtual attributes for Containers and friends (performance changes) Aug 1, 2023
@miq-bot miq-bot removed the wip label Aug 1, 2023
@kbrock
Copy link
Member Author

kbrock commented Aug 1, 2023

WIP: confusing CI values

I'm very confused by the failure. This works locally and I can't imagine where this could be going wrong. All test failures are the same. (I wrote the specs at the same time)

The spec(s) only shows partial SQL, but it looks like where(:id => subject.id) is sending the container_node.id as text instead of a numeric.

# rspec ./spec/models/container_group_spec.rb:53
# rspec ./spec/models/container_group_spec.rb:61
# rspec ./spec/models/container_node_spec.rb:50 <==

PG::UndefinedFunction: ERROR:  operator does not exist: bigint = text
LINE 1: ...OM "container_nodes" WHERE "container_nodes"."id" = $1 ORDER...

Again, the sql generated locally is working fine.

Also interesting:

subj = described_class.where(:id => container_group.id)
expect(subj.first.ready_condition_status).to eq("None") # works
expect(subj.select(:ready_condition_status).first.ready_condition_status).to eq("None") # failure

@kbrock kbrock changed the title Virtual attributes for Containers and friends (performance changes) [WIP] Virtual attributes for Containers and friends (performance changes) Aug 1, 2023
@miq-bot miq-bot added the wip label Aug 1, 2023
Copy link
Member

Choose a reason for hiding this comment

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

I think this file was accidentally committed to this PR.

similar to 773fea3

already introduced indexes, so this is just ruby changes

|     ms |query | qry ms |     rows |` comments`
|    ---:|  ---:|   ---:|      ---:|  ---
|  347.2 |   32 |  53.2 |      128 |` /container_node/report_data#node-index`
|  308.8 |   11 |  10.3 |       28 |` /container_node/report_data#node-after`
|  11.1% |  66% | 80.6% |      78% | diff

|     ms |query | qry ms |     rows |` comments`
|    ---:|  ---:|   ---:|      ---:|  ---
|  428.0 |   32 | 123.2 |       88 |` /container_group/report_data#group-index`
|  336.5 |   11 |  65.5 |       28 |` /container_group/report_data#group-after`
|  21.4% |  66% | 46.8% |      68% | diff
add ContainerImage#display_registry delegate to the full_name

|     ms |query | qry ms |     rows |` comments`
|    ---:|  ---:|   ---:|      ---:|  ---
|  424.3 |   51 |  98.7 |       48 |` /container_image/report_data`
|  345.1 |   11 |  34.4 |       28 |` /container_image/report_data`
|   18.7%|   78%|  65.1%|       42%| diff

Localization of "Unknown image source" has been removed and moved
into the UI layer
@kbrock
Copy link
Member Author

kbrock commented Oct 20, 2023

update:

  • rebase
  • fixed extra file committed (thnx fry)

@miq-bot
Copy link
Member

miq-bot commented Oct 31, 2023

Checked commits kbrock/manageiq@53e9556~...4cb708a with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
8 files checked, 0 offenses detected
Everything looks fine. 👍

@@ -46,7 +46,7 @@ class ContainerImage < ApplicationRecord
serialize :exposed_ports, Hash
serialize :environment_variables, Hash

virtual_column :display_registry, :type => :string
virtual_delegate :display_registry, :to => "container_image_registry.full_name", :allow_nil => true, :type => :string
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this returns nil? Is there a presentation-side change where it will say "Unknown image source", or does it leave it blank?

reg = ContainerImageRegistry.new(:name => "docker.io", :host => "docker.io")
expect(reg.full_name).to eq("docker.io")
describe "#full_name" do
it "works with no port" do
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a spec where the port is "" since there's a case for that?

@Fryguy
Copy link
Member

Fryguy commented Nov 1, 2023

Overall LGTM - one real question, and some specs are failing, but maybe they go away with a rebase since Ruby 2.7 is gone now.

@miq-bot miq-bot added the stale label Feb 5, 2024
@miq-bot
Copy link
Member

miq-bot commented Feb 5, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2024

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Jun 10, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Sep 16, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants