Skip to content

restartcheck: add NILRT ARM kernel version detection - #47610

Merged
rallytime merged 2 commits into
saltstack:developfrom
10ne1:dev/aratiu/nilrt-restartcheck-arm-kernel-version
Jul 16, 2018
Merged

restartcheck: add NILRT ARM kernel version detection#47610
rallytime merged 2 commits into
saltstack:developfrom
10ne1:dev/aratiu/nilrt-restartcheck-arm-kernel-version

Conversation

@10ne1

@10ne1 10ne1 commented May 11, 2018

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds the capability to detect ARM kernel versions to restartcheck on NI Linux RT platforms.

Tests written?

No

Commits signed with GPG?

No

@cachedout

Copy link
Copy Markdown
Contributor

@jirikotlin is this something you could help us review?

@10ne1
10ne1 force-pushed the dev/aratiu/nilrt-restartcheck-arm-kernel-version branch from 07f186e to 830f9b7 Compare May 23, 2018 14:46
@rallytime

Copy link
Copy Markdown
Contributor

@isbm You came up on GitHub's reviewers suggestions list, so I've requested a review from you here. Would you mind taking a look?

@rallytime
rallytime requested a review from isbm May 31, 2018 18:45
Comment thread salt/modules/restartcheck.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, very nice! if distribution_i_use_at_my_office(): anyone? 😉

@10ne1 since seems NILRT does not have a proper versioning, then can we come up with something generic instead, like property checker etc, which will do decision based on that (say, "foo is installed", "bar is located there" etc) instead of hardcoding "older version" or "even_older_version" right into the code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I can modify the distro rootfs without breaking backwards compatibility such that the lsb_release DISTRIB_ID field reflects the difference between these older/newer versions and that would automatically get picked up by the existing lsb_distrib_id grain, so then we'd just have to test for grains[lsb_distrib_id] == 'nilrt'.

Are you ok with this approach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@isbm A collegue of mine posted a PR implementing my suggestion, if that gets merged we can test the lsb grain in this one #47951

@isbm

isbm commented Jun 2, 2018

Copy link
Copy Markdown
Contributor

@rallytime seems your bot works very well! Last time I was reviewing this indeed. You should also probably take to the account number of comments posted by a reviewer... 😆

Comment thread salt/modules/restartcheck.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you split this doc string into 3 lines instead of 1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rallytime Yes, done.

@10ne1
10ne1 force-pushed the dev/aratiu/nilrt-restartcheck-arm-kernel-version branch from 830f9b7 to b944c86 Compare June 8, 2018 14:20
@10ne1

10ne1 commented Jun 8, 2018

Copy link
Copy Markdown
Contributor Author

@isbm @rallytime I've rebased on top of the develop branch and removed _is_older_nilrt in favor of the lsb_distrib_id grain which recently got fixed by #47951

@10ne1
10ne1 force-pushed the dev/aratiu/nilrt-restartcheck-arm-kernel-version branch from b944c86 to 4eafcf6 Compare June 8, 2018 14:25
@10ne1
10ne1 force-pushed the dev/aratiu/nilrt-restartcheck-arm-kernel-version branch 2 times, most recently from 8d0dbb1 to 9e9b4e4 Compare July 10, 2018 08:01
Detecting the NXG ARM kernel version is easy because the kernel is
installed the same way as on x64, only uncompressed (uImage).

For older ARM NILRT however on-disk kernel version detection is harder
because the kernel is compressed and stored inside an u-boot "itb"
image alongside it's device tree, ramdisk and a bootscript, so we need
to extract it and decompress.

Make the distinction between older vs newer NILRT useing the new
lsb_distrib_id grain which recently got repurposed in NILRT.

Signed-off-by: Ioan-Adrian Ratiu <adrian.ratiu@ni.com>
@10ne1

10ne1 commented Jul 10, 2018

Copy link
Copy Markdown
Contributor Author

Kind reminder. Ping?

@isbm isbm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@10ne1 oh, sorry! Thanks for the reminder, it was really needed. Yeah, looks better. I still don't like that you need to re-check the path to the image and in my personal opinion I would move it away to a separate function, like _get_kernel_path. If you would be kind to do that, go ahead.

Otherwise you can leave it as is for now.

@rallytime
rallytime merged commit a33e449 into saltstack:develop Jul 16, 2018
@10ne1
10ne1 deleted the dev/aratiu/nilrt-restartcheck-arm-kernel-version branch July 16, 2018 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants