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

Self-assembling NTP Zone #2900

Closed
wants to merge 5 commits into from
Closed

Self-assembling NTP Zone #2900

wants to merge 5 commits into from

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Apr 21, 2023

Fixes #2885

@smklein smklein requested a review from citrus-it April 21, 2023 05:37
Copy link
Contributor

@citrus-it citrus-it left a comment

Choose a reason for hiding this comment

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

I had a few comments on the manifest and script, but mostly notes or ones you can take or leave (github doesn't seem to have a way to flag each one that way).

Comment on lines 37 to 44
ALL_LINKS=( $ALL_LINKS )

for LINK in "${ALL_LINKS[@]}"; do
ipadm show-addr "$LINK/ll" || ipadm create-addr -t -T addrconf "$LINK/ll"
done
ipadm show-addr "$DATALINK/ll" || ipadm create-addr -t -T addrconf "$DATALINK/ll"
ipadm show-addr "$DATALINK/omicron6" || ipadm create-addr -t -T static -a "$LISTEN_ADDR" "$DATALINK/omicron6"
route get -inet6 default -inet6 "$GATEWAY" || route add -inet6 default -inet6 "$GATEWAY"
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually things like this would only be done on service start and not on other methods such as stop and refresh, which will be run if somebody uses svcadm or if chrony were to crash. Since they're all conditional there won't be a problem, so feel free to ignore.

done
ipadm show-addr "$DATALINK/ll" || ipadm create-addr -t -T addrconf "$DATALINK/ll"
ipadm show-addr "$DATALINK/omicron6" || ipadm create-addr -t -T static -a "$LISTEN_ADDR" "$DATALINK/omicron6"
route get -inet6 default -inet6 "$GATEWAY" || route add -inet6 default -inet6 "$GATEWAY"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
route get -inet6 default -inet6 "$GATEWAY" || route add -inet6 default -inet6 "$GATEWAY"
route -n get -inet6 default "$GATEWAY" || route add -inet6 default "$GATEWAY"

-n so it does not try and resolve numbers to names, and the -inet6 flag was duplicated.

ipadm show-addr "$LINK/ll" || ipadm create-addr -t -T addrconf "$LINK/ll"
done
ipadm show-addr "$DATALINK/ll" || ipadm create-addr -t -T addrconf "$DATALINK/ll"
ipadm show-addr "$DATALINK/omicron6" || ipadm create-addr -t -T static -a "$LISTEN_ADDR" "$DATALINK/omicron6"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: over 80 columns

Boundary: $boundary
Template: $template
Config: $file
NTP Service Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

The cat <<- construct requires the body to be indented by tabs, not spaces.
It doesn't matter much as the output will just end up indented in the service log file.

illumos script style is tabs, 80 columns etc, and is what I'd used, but since this is in omicron that style probably takes precedence.

Comment on lines +82 to +83
<propval name='gateway' type='astring' value='unknown' />
<propval name='listen_addr' type='astring' value='unknown' />
Copy link
Contributor

Choose a reason for hiding this comment

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

These could be type='net_address_v6' (assuming they are always IPv6 addresses)

@@ -14,7 +14,7 @@
type="service"
version="1">

<create_default_instance enabled="false" />
<create_default_instance enabled='true' />
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're changing to single quotes here, perhaps make it consistent throughout?

@karencfv
Copy link
Contributor

karencfv commented Nov 22, 2023

Hey there! I'm taking over this bit of work from @smklein :) (spoke about this already)

@citrus-it, I think I've addressed your comments, but so much has changed since your last review that it's probably a good idea to go over it again if you don't mind?

🤞 the integration tests pass after the giant merge.

@karencfv karencfv marked this pull request as draft November 22, 2023 23:54
@karencfv
Copy link
Contributor

I'm not certain we still need the all_links property anymore since &installed_zone.links() is not returning anything. I'm testing to see what happens if I remove it.

@karencfv karencfv self-assigned this Nov 23, 2023
@davepacheco
Copy link
Collaborator

I'm not sure how close this is to landing, but I've rewritten quite a lot of this code in #4466. I think it would be a lot easier to land this PR after #4466 (and I'd be happy to help with the merge) than the other way around, if that's okay.

@karencfv
Copy link
Contributor

I'm not sure how close this is to landing, but I've rewritten quite a lot of this code in #4466. I think it would be a lot easier to land this PR after #4466 (and I'd be happy to help with the merge) than the other way around, if that's okay.

@davepacheco of course! This has been open for a while anyway. I'll be on stand-by then

@karencfv
Copy link
Contributor

karencfv commented Mar 7, 2024

Superseded by: #5168

This PR was really far behind, it was easier to just open up a new one

@karencfv karencfv closed this Mar 7, 2024
@karencfv karencfv deleted the self-assembling-ntp branch August 28, 2024 05:19
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.

Convert NTP to be self-assembling
4 participants