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

Proxmox extention for foreman_bootdisk #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Manisha15
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@Manisha15 Manisha15 changed the title Proxmox extention for foreman_bootdisk WIP:Proxmox extention for foreman_bootdisk Mar 11, 2020
@Manisha15 Manisha15 force-pushed the proxmox_extension branch 2 times, most recently from bf1c6dc to eafb56c Compare March 30, 2020 07:29
@Manisha15 Manisha15 changed the title WIP:Proxmox extention for foreman_bootdisk Proxmox extention for foreman_bootdisk Mar 30, 2020
@sbernhard
Copy link
Contributor

Thanks Manisha. I guess, it would make sense that @tristanrobert (foreman fog proxmox) should also have a look at this.

Copy link
Contributor

@m-bucher m-bucher left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to add tests.
However, I saw that the new_vm-test only tests if foreman_fog_proxmox does not change the attributes upon calling the new_server_vm() (and if it uses server-virtualization).
This is a pretty small coverage.
Any chance you could extend this (without mocking a complete proxmox-server 😉 ).

It would be good to have a test for iso_upload() and iso_attach() as well.

@Manisha15
Copy link
Contributor Author

Thanks for taking the time to add tests.
However, I saw that the new_vm-test only tests if foreman_fog_proxmox does not change the attributes upon calling the new_server_vm() (and if it uses server-virtualization).
This is a pretty small coverage.
Any chance you could extend this (without mocking a complete proxmox-server ).

It would be good to have a test for iso_upload() and iso_attach() as well.

Thank you for reviewing. I have added tests for iso_upload and iso_attach. Can you please review again.

@sbernhard
Copy link
Contributor

[test foreman_bootdisk]

1 similar comment
@sbernhard
Copy link
Contributor

[test foreman_bootdisk]

Copy link
Contributor

@m-bucher m-bucher left a comment

Choose a reason for hiding this comment

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

Unit-tests always make me dizzy 💫

Right now I can remove the iso_attach() and iso_upload() methods and the tests run through successfully 😆 .
But the vmware implementation also does not test if the iso_upload()-method is actually there and calls the backend with a specific method and parameters.

Therefore, it is probably not worth the effort to implement it for proxmox right now 😄

If you fix the typo in the new test, I am ok with the current state.

end

test 'provisioning a host with provision method bootdisk should upload iso' do
@cr.expects(:iso_upload)
@host.send(:setIsoImage)
end

test 'provisioning a host with provision method bootdisk in proxmox should attach iso' do
@proxmox_cr.expects(:iso_upload)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean :iso_attach, here.

@@ -4,6 +4,9 @@
require 'webmock/minitest'
require 'webmock'

FactoryBot.definition_file_paths << File.join(File.dirname(__FILE__), 'factories')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FactoryBot.definition_file_paths << File.join(File.dirname(__FILE__), 'factories')
FactoryBot.definition_file_paths << File.join(__dir__, 'factories')

@sbernhard
Copy link
Contributor

[test foreman_bootdisk]

1 similar comment
@sbernhard
Copy link
Contributor

[test foreman_bootdisk]

@sbernhard
Copy link
Contributor

Looks like the tests are running but some rubocop issues occured in the proxmox extension. Can you have a look at them @Manisha15 ?

See https://ci.theforeman.org/job/foreman_bootdisk-pull-request/36/database=postgresql,label=fast,ruby=2.6/console

@Manisha15 Manisha15 force-pushed the proxmox_extension branch 5 times, most recently from b247129 to c78d8a0 Compare April 6, 2020 07:11
@sbernhard
Copy link
Contributor

[test foreman_bootdisk]

@Manisha15 Manisha15 marked this pull request as ready for review October 26, 2023 13:41
@Manisha15
Copy link
Contributor Author

@evgeni @lzap Can you please review this again? The tests are green now.

@@ -45,6 +45,7 @@ jobs:
- name: Setup Bundler
run: |
echo "gem 'foreman_bootdisk', path: './foreman_bootdisk'" > bundler.d/foreman_bootdisk.local.rb
echo "gem 'foreman_fog_proxmox'" >> bundler.d/foreman_bootdisk.local.rb
Copy link
Member

Choose a reason for hiding this comment

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

I would have expected this is not required, given you also edited Gemfile? (not blocking, just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, now it is not required.

Copy link
Member

Choose a reason for hiding this comment

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

FYI: in our common CI we use gemfile.d/*.rb. For our reusable GHA it's here and Jenkins implements a similar thing.

Then in Gemfile you can use eval_gemfile:

group :test do
  eval_gemfile 'gemfile.d/proxmox.rb'
end

This means you only specify >= 0.14.2 once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this change. But how will it find the gemfile.d/proxmox.rb as it's not there on foreman side as well as in foreman_bootdisk gemfile.d ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl This also fails the rubocop as it cannot find the gemfile.d/proxmox.rb.

Copy link
Member

Choose a reason for hiding this comment

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

Added this change. But how will it find the gemfile.d/proxmox.rb as it's not there on foreman side as well as in foreman_bootdisk gemfile.d ?

Foreman doesn't care about Gemfile, so it shouldn't matter.

@ekohl This also fails the rubocop as it cannot find the gemfile.d/proxmox.rb.

Because you didn't use git add so it's not in the repository.

@evgeni
Copy link
Member

evgeni commented Nov 8, 2023

I am neither a bootdisk nor a proxmox expert, but this doesn't read crazy to me :)

@evgeni
Copy link
Member

evgeni commented Nov 8, 2023

fixing the RPM build in #149

@evgeni
Copy link
Member

evgeni commented Nov 8, 2023

Now it skipped the Proxmox tests 😞

Sorry for the bad idea

@Manisha15 Manisha15 force-pushed the proxmox_extension branch 2 times, most recently from dbe30c0 to 29a3d1e Compare November 8, 2023 15:22
@Manisha15
Copy link
Contributor Author

Now it skipped the Proxmox tests 😞

Sorry for the bad idea

changed it again, and now tests are running and green.

@@ -45,6 +45,7 @@ jobs:
- name: Setup Bundler
run: |
echo "gem 'foreman_bootdisk', path: './foreman_bootdisk'" > bundler.d/foreman_bootdisk.local.rb
echo "gem 'foreman_fog_proxmox'" >> bundler.d/foreman_bootdisk.local.rb
Copy link
Member

Choose a reason for hiding this comment

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

FYI: in our common CI we use gemfile.d/*.rb. For our reusable GHA it's here and Jenkins implements a similar thing.

Then in Gemfile you can use eval_gemfile:

group :test do
  eval_gemfile 'gemfile.d/proxmox.rb'
end

This means you only specify >= 0.14.2 once.

Comment on lines 26 to 29
config_hash = { ide2: "#{volume.volid},media=cdrom" }
server.update(config_hash)
server.update({ boot: "order=ide2;#{disks}" })
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK the latest rubocop-performance will flag this and tell you to simplify to regular assignments:

Suggested change
config_hash = { ide2: "#{volume.volid},media=cdrom" }
server.update(config_hash)
server.update({ boot: "order=ide2;#{disks}" })
server[:ide2] = "#{volume.volid},media=cdrom"
server[:boot] = "order=ide2;#{disks}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's a server object and not a hash so this assignment is not possible and it throw error NoMethodError (undefined method []=' for #Fog::Proxmox::Compute::Server:0x0000564003398f20)`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And we want to first add the cdrom (ide2) and update the server and then update the boot order because if we update the server with both cdrom and boot order, it takes wrong boot order. It tries to update the attributes alphabetically and without adding ide2 it cannot update boot order properly.

server.update(config_hash)

# cdrom will be ejected on next power off
server.detach('ide2')
Copy link
Member

Choose a reason for hiding this comment

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

Given the repetition, would it be good to declare ide2 as a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ide2 is a cdrom and for attachment and detachment it has different values. In iso detach function, first it is ejected, thus, set to none and detach action takes place which will be executed when the vm is powered off. So, it cannot be set as a constant.

Copy link
Member

Choose a reason for hiding this comment

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

But it's hardcoded in the code, so it already is. I don't see why you can't use CDROM_VOLUME = 'ide2' and then later server.detach(CDROM_VOLUME).


# delete the iso file from proxmox server
storage = storages(server.node_id, 'iso')[0]
volume = storage.volumes.detect { |v| v.volid.include? volid }
Copy link
Member

Choose a reason for hiding this comment

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

When would this volume ID not be exactly equal? If the volume IDs are integers I'd imagine this also end up being 123'.include?('12') so

Suggested change
volume = storage.volumes.detect { |v| v.volid.include? volid }
volume = storage.volumes.detect { |v| v.volid == volid }

Also, previously you already used volid = server.volumes.get("ide2").volid so why can't you just do:

server.volumes.get('ide2').destroy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to destroy the disk 'ide2' as the operation is not possible when vm is powered on. We just want to destroy the volume so that the iso files are not piled up in proxmox server.

Copy link
Member

Choose a reason for hiding this comment

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

So volume = storage.volumes.detect { |v| v.volid.include? volid } returns a different object than server.volumes.get("ide2"). That would surprise me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, server.volumes are disks of the server so it returns the disks, Fog::Proxmox::Compute::Disks and storage.volumes are volumes so it returns Fog::Proxmox::Compute::Volumes. You can remove the volumes without caring about server but you cannot remove the disk without powering off the server (hotplug issue).

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.

10 participants