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

(RE-15156) Add redhat-9-arm64 support #327

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

yachub
Copy link
Contributor

@yachub yachub commented Sep 8, 2023

I'm not sure about all the places that the abs and vmpooler platform data keys are used, but was a bit confused because the redhat8-AARCH64 specifies the x86 template name redhat-8-x86_64, is that a mistake?

'template' => 'redhat-8-x86_64',

So for the new platform's vmpooler key I used the expected name that we would use in vmpooler for that platform, is that correct? Or should that key just be omitted?

EDIT: Ah it looks like those keys related to the hypervisor gem. The above definitely seems like a bug per:

> beaker-hostgenerator redhat8-AARCH64 --hypervisor vmpooler
---
HOSTS:
  redhat8-AARCH64-1:
    platform: el-8-aarch64
    template: redhat-8-x86_64
    hypervisor: vmpooler
    roles:
    - agent
CONFIG:
  pooling_api: https://vmpooler-prod.k8s.infracore.puppet.net/

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (da8ac9a) 0.73% compared to head (0bca3ce) 0.73%.

❗ Current head 0bca3ce differs from pull request most recent head 86da134. Consider uploading reports for the commit 86da134 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #327      +/-   ##
=========================================
- Coverage    0.73%   0.73%   -0.01%     
=========================================
  Files          15      15              
  Lines        1765    1776      +11     
=========================================
  Hits           13      13              
- Misses       1752    1763      +11     
Files Changed Coverage Δ
lib/beaker-hostgenerator/data.rb 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@ekohl
Copy link
Member

ekohl commented Sep 8, 2023

For RHEL I haven't looked at correct generation yet. As you noticed, the data was inconsistent. Like redhat7-AARCH64 uses centos-7-arm64 in abs.

Ideally we would generate the ABS template in https://github.com/voxpupuli/beaker-hostgenerator/blob/master/lib/beaker-hostgenerator/hypervisor/abs.rb and vmpooler in https://github.com/voxpupuli/beaker-hostgenerator/blob/master/lib/beaker-hostgenerator/hypervisor/vmpooler.rb. Then it would be reduced to just listing the versions in generate_os_info.

The biggest problem is that Vox Pupuli doesn't use ABS or vmpooler so we really don't know what's available and what makes sense.

@joshcooper
Copy link
Contributor

Seems like a bug to me too. IMO BHG should always return a template/platform name that is consistent with the key (BHG string):

We do cross compile on some platforms, but the decision about whether to cross-compile and which build host to use lives in the platform definition for the vanagon project, for example: https://github.com/puppetlabs/puppet-runtime/blob/6c8bbc664aea36cbadc12ab0d62773250faf70f1/configs/platforms/sles-12-ppc64le.rb#L31-L32

In other words, I don't see a use case for BHG ever returning an Intel-based template for an ARM-based key.

@yachub yachub merged commit 5b257ab into voxpupuli:master Sep 11, 2023
@joshcooper
Copy link
Contributor

Not sure why, but I'm having trouble getting this working. This line is causes an error if platform is not defined:

https://github.com/voxpupuli/beaker-hostgenerator/blob/013dab5a0ab39c5d1b7383ec1265c382eb325dac/lib/beaker-hostgenerator/hypervisor/abs.rb#L27C39-L27C50

$ bx beaker-hostgenerator redhat9-ARM64a
bundler: failed to load command: beaker-hostgenerator (/home/josh/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/bin/beaker-hostgenerator)
Traceback (most recent call last):
...
	 6: from /home/josh/work/beaker-hostgenerator/bin/beaker-hostgenerator:10:in `<top (required)>'
	 5: from /home/josh/work/beaker-hostgenerator/lib/beaker-hostgenerator/cli.rb:174:in `execute!'
	 4: from /home/josh/work/beaker-hostgenerator/lib/beaker-hostgenerator/cli.rb:168:in `execute'
	 3: from /home/josh/work/beaker-hostgenerator/lib/beaker-hostgenerator/generator.rb:30:in `generate'
	 2: from /home/josh/work/beaker-hostgenerator/lib/beaker-hostgenerator/generator.rb:30:in `each'
	 1: from /home/josh/work/beaker-hostgenerator/lib/beaker-hostgenerator/generator.rb:52:in `block in generate'
/home/josh/work/beaker-hostgenerator/lib/beaker-hostgenerator/hypervisor/vmpooler.rb:22:in `generate_node': undefined method `gsub' for nil:NilClass (NoMethodError)

I get the same when passing abs or vmpooler as the hypervisor

@yachub
Copy link
Contributor Author

yachub commented Oct 2, 2023

@joshcooper Following precedent of most recently added platforms I used "AARCH64" instead of "ARM64". Does that work for you?

> beaker --version
      wWWWw
      |o o|
      | O |  5.3.1!
      |(")|
     / \X/ \
    |   V   |
    |   |   |
    
> beaker-hostgenerator -v
2.4.0

> beaker-hostgenerator redhat9-AARCH64a --hypervisor abs
---
HOSTS:
  redhat9-AARCH64-1:
    platform: el-9-aarch64
    template: redhat-9-arm64
    hypervisor: abs
    roles:
    - agent
CONFIG: {}

@ekohl
Copy link
Member

ekohl commented Oct 2, 2023

Still, would be good to either silently ignore unknown architectures or fail with a clear error instead of a stack trace.

@joshcooper
Copy link
Contributor

Still, would be good to either silently ignore unknown architectures or fail with a clear error instead of a stack trace.

yeah that could be handled better. Took me a bit to understand what was going on. I also run into errors like that when trying to run beaker tests and I've forgotted to do bundle update to pull in a version of BHG that recognizes the new platform.

I used "AARCH64" instead of "ARM64". Does that work for you?

Yeah I think it's good for the architecture to be consistent for all versions of the same OS. As for why we use different names for one OS vs another, I can only refer to https://twitter.com/lorenc_dan/status/1699794514586763408:

F5biGZgbcAA9RZ9

@ekohl
Copy link
Member

ekohl commented Oct 3, 2023

$ bx beaker-hostgenerator redhat9-ARM64a

Funny, I use be as the bundle exec alias.

Still, would be good to either silently ignore unknown architectures or fail with a clear error instead of a stack trace.

#337

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.

3 participants