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

aixPB: Modify order of executition to have requirements first #1695

Merged
merged 6 commits into from
Dec 4, 2020
Merged

aixPB: Modify order of executition to have requirements first #1695

merged 6 commits into from
Dec 4, 2020

Conversation

aixtools
Copy link
Contributor

@aixtools aixtools commented Nov 20, 2020

Checklist

@karianna karianna added the bug label Nov 24, 2020
@karianna karianna added this to the November 2020 milestone Nov 24, 2020
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

Some comments as requested - hopefully once we've got this in, adding others into the top level playbook will be easier

- xlc_v13
- xlc_v16
- openssl
# AIX configuration
- aixfs
Copy link
Member

Choose a reason for hiding this comment

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

Why is this moved to after the XLC/X11 installations? Wouldn't moving this cause problems if the file systems weren't large enough to install XLC/X11? I think that's why it was at the start previously.

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 is before xlc installation - it WAS after syslogs - which is a configuration change.

What I am trying to do is get AIX BOS installation first, then AIX configuration changes (that does not require any non-BOS software) and then start looking at licensed software.

As the xlc roles use, potentially, the unarchive: module - the latest update has the role: yum before the xlc role calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it looks like it is after, but it is not. No idea how the diff comes up with this.

Further - as to large enough - installp enlarges the filesystem if more space is needed.

Copy link
Member

Choose a reason for hiding this comment

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

What I am trying to do is get AIX BOS installation first, then AIX configuration changes (that does not require any non-BOS software) and then start looking at licensed software.

OK That reasoning is sound, but it might be worth explcitly saying that so that others modifying the file in the future adhere to the same policies

I agree, it looks like it is after, but it is not. No idea how the diff comes up with this.

Looks ok now - it's showing where it should be based on your comments

Further - as to large enough - installp enlarges the filesystem if more space is needed.

Yep that's fair, but I'd suggest adding that as a comment so it's clear to anyone modifying the playbook in the future

ansible/playbooks/AdoptOpenJDK_AIX_Playbook/main.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@Haroon-Khel Haroon-Khel left a comment

Choose a reason for hiding this comment

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

See #1704 (comment)

Just to reiterate, the yum role should run before any role which involves decompressing and unzipping packages (such as x11, xlc13 and xlc16), as it installs tar and unzip. I thought this change had already gone in some time ago, but it seems im mistaken. I ran into the error while testing the linked pr, but thought the change would be more appropriate here

fatal: [p9-aix1-ojdk06.osuosl.org]: FAILED! => {"changed": false, "msg": "Failed to find handler for \"/root/.ansible/tmp/ansible-tmp-1606411573.995723-14158-56646512312999/source\". Make sure the required command to extract the file is installed. Command \"/usr/bin/tar\" detected as tar type None. GNU tar required. Command \"unzip\" not found."}

@aixtools
Copy link
Contributor Author

See #1704 (comment)

Just to reiterate, the yum role should run before any role which involves decompressing and unzipping packages (such as x11, xlc13 and xlc16), as it installs tar and unzip. I thought this change had already gone in some time ago, but it seems im mistaken. I ran into the error while testing the linked pr, but thought the change would be more appropriate here

fatal: [p9-aix1-ojdk06.osuosl.org]: FAILED! => {"changed": false, "msg": "Failed to find handler for \"/root/.ansible/tmp/ansible-tmp-1606411573.995723-14158-56646512312999/source\". Make sure the required command to extract the file is installed. Command \"/usr/bin/tar\" detected as tar type None. GNU tar required. Command \"unzip\" not found."}

moved per 104de6a

@aixtools aixtools requested a review from Haroon-Khel November 27, 2020 17:22
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

If we add in some comments to make it clear why the items have been sequenced the way we have this is good to merge. Let's aim to get it in Monday at the latest :-)

ansible/playbooks/AdoptOpenJDK_AIX_Playbook/main.yml Outdated Show resolved Hide resolved
- xlc_v13
- xlc_v16
- openssl
# AIX configuration
- aixfs
Copy link
Member

Choose a reason for hiding this comment

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

What I am trying to do is get AIX BOS installation first, then AIX configuration changes (that does not require any non-BOS software) and then start looking at licensed software.

OK That reasoning is sound, but it might be worth explcitly saying that so that others modifying the file in the future adhere to the same policies

I agree, it looks like it is after, but it is not. No idea how the diff comes up with this.

Looks ok now - it's showing where it should be based on your comments

Further - as to large enough - installp enlarges the filesystem if more space is needed.

Yep that's fair, but I'd suggest adding that as a comment so it's clear to anyone modifying the playbook in the future

@Haroon-Khel
Copy link
Contributor

Haroon-Khel commented Dec 1, 2020

Ive tested these changes, and those of #1704, on ojdk06 (with the requested changes). The playbook ran clean. A build of jdk11 hotspot ran fine too. Running an openjdk sanity test resulted in one failure:

OLD.getZoneInfoOld()[1]=518
OLD.getZoneInfoOld()[2]=386
OLD.getZoneInfoOld()[3]=276
OLD.getAvailableIDs()=998, total=628
OLD.getAliasTable()=486, total=228
OLD.TotalTZ()=66 (ms)
NEW.getTimeZone()[1]=21
NEW.getTimeZone()[2]=1
NEW.getTimeZone()[3]=1
NEW.getAvailableIDs()=17, total=629
NEW.getAliasTable()=86, total=229
NEW.TotalTZ()=0 (ms)
STDERR:
javazic: warning: found last rules for Morocco inconsistent.
javazic: warning: found last rules for Morocco inconsistent.
javazic: warning: found last rules for Eire inconsistent.
java.lang.RuntimeException:   FAILED:  availableIds don't match
        at TestZoneInfo310.main(TestZoneInfo310.java:163)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:312)
        at java.base/java.lang.Thread.run(Thread.java:836)

JavaTest Message: Test threw exception: java.lang.RuntimeException
JavaTest Message: shutting down test


TEST RESULT: Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: FAILED: availableIds don't match

Ive been informed that the test failure is related to a common bug, and not to do with these changes. Need to investigate this more.

@Haroon-Khel
Copy link
Contributor

Haroon-Khel commented Dec 1, 2020

@smlambert, as mentioned in the team meeting

@smlambert
Copy link
Contributor

Background:
The TestZoneInfo310 is most likely the result of using test material that does not match the TZ updates of the JVM. It is the same issue reported here: adoptium/aqa-tests#1356 (which was resolved by the adding RELEASE_TAG support, in other words telling the test pipeline which openjdk repo tag to use to pull test material from, so that test material matches the JVM). In Grinders, where test pipeline is not passed a RELEASE_TAG, it will assume pull test material from tip (openjdk master branch).

As discussed in meeting today, you can presume its non-blocking, but it is interesting that there is a skew. Are you using nightly or releases builds(value of SDK_RESOURCE parameter) in your Grinder jobs?

@Haroon-Khel
Copy link
Contributor

@smlambert Im running the tests directly on the machine itself, since it is not in jenkins. Im using binaries from the adopt website, https://adoptopenjdk.net/releases.html?variant=openjdk11&jvmVariant=openj9

@Haroon-Khel
Copy link
Contributor

@aixtools Seeing as this test failure is non blocking, this pr is ready to be merged (after the requested changes have been put in)

* move aixfs configuration to precede yum - to ensure sufficient FS space for
  the yum installed packages.
* move XLC installation to after yum processing to ensure Ansible unarchive
  module requirements are available.
@aixtools
Copy link
Contributor Author

aixtools commented Dec 3, 2020

I hope I saw, and resolved everyone's requests. If I missed something - my apology in advance.

@aixtools aixtools requested review from Haroon-Khel and sxa December 3, 2020 14:05
Copy link
Member

@sxa sxa 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 the changes - LGTM other than the linter issue with extra spaces at the end of one of the lines which should be resolved before merging

@aixtools
Copy link
Contributor Author

aixtools commented Dec 4, 2020

Trailing space removed. Thx for the review.

@aixtools
Copy link
Contributor Author

aixtools commented Dec 4, 2020

I'll add one more change - although I feel it is wrong to add something that is likely not used - and definitely not needed. But getting Ansible to modify unarchive: to not fail if unzip and gtar are not present is probably impossible.

…lation

because it might need the Ansible unarchive: module.
Also, number the role grouping to imporve recognition of 'sections'
@Haroon-Khel
Copy link
Contributor

Haroon-Khel commented Dec 4, 2020

@aixtools What if instead of using the unarchive module, we use the copy module to transfer the package over, and then use the shell module to run something like tar xvf {{ path to x11 package }} ? Assuming tar is already present on the machine before the yum role.

@aixtools
Copy link
Contributor Author

aixtools commented Dec 4, 2020

@aixtools What if instead of using the unarchive module, we use the copy module to transfer the package over, and then use the shell module to run something like tar xvf {{ path to x11 package }} ? Assuming tar is already present on the machine before the yum role.

Something like that would be my suggestion - but that can come later, i.e., the role calls can be moved when that is completed - as part of that PR perhaps. For now, my goal is to document why A or B is done, and have it working - in it's current shape.

I have also understood CI as constant improvement

Copy link
Contributor

@Haroon-Khel Haroon-Khel left a comment

Choose a reason for hiding this comment

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

LGTM

@Haroon-Khel Haroon-Khel merged commit 0bf2053 into adoptium:master Dec 4, 2020
karianna added a commit that referenced this pull request Dec 4, 2020
* Fixes from Markdown and Yaml linters + spelling typos

* ansible: refresh macOS test machines + playbook patches (#1665)

* Ansible: refresh macOS test machines + playbook patches

* linter fixes

* Update main.yml

* Update MacOSX.yml

* Update main.yml

* Update main.yml

* Update main.yml

* Update main.yml

* Update MacOSX.yml

* Update MacOSX.yml

* pbTests: Fix -nh option on the VPC help screen (#1735)

Signed-off-by: Stewart X Addison <[email protected]>

* Set remote_tmp for Ansible (#1736)

* aixPB: Remove packages already installed by yum (#1704)

* aixPB: Remove packages already installed via yum: statements

* aixPB: Add additional tag 'yum' to a task

* aixPB: Add additional tag 'yum' to a task

* aixPB: Modify order of executition to have requirements first (#1695)

* aixPB: Modify order of so-called role executition to have requirements first

* aixPB: GNU software needed for some of the Ansible modules used in other plays

* aixPB: Remove spurious comments

* aixPB: Organize (and document/motivate) six groups of roles.
* move aixfs configuration to precede yum - to ensure sufficient FS space for
  the yum installed packages.
* move XLC installation to after yum processing to ensure Ansible unarchive
  module requirements are available.

* aixPB: Remove trailing space

* aixPB: move X11 (AIX BOS) installation check to after OSS core installation
because it might need the Ansible unarchive: module.
Also, number the role grouping to imporve recognition of 'sections'

* doc: typo corrections and header modifications (#1734)

* Fixes from Markdown and Yaml linters + spelling typos

* fix to URL from Stewart review

Co-authored-by: George Adams <[email protected]>
Co-authored-by: Stewart X Addison <[email protected]>
Co-authored-by: Will Parker <[email protected]>
Co-authored-by: Michael Felt <[email protected]>
@karianna karianna modified the milestones: November 2020, December 2020 Dec 4, 2020
@aixtools aixtools deleted the aix-pb2-main branch December 10, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants