TritonDataCenter / smartos-live

For more information, please see http://smartos.org/ For any questions that aren't answered there, please join the SmartOS discussion list: https://smartos.topicbox.com/groups/smartos-discuss
1.57k stars 246 forks source link

OS-8209 piadm should allow injecting loader.*.local #958

Closed sjorge closed 4 years ago

sjorge commented 4 years ago

After this change piadm will try to symlink loader.conf.local and loader.rc.local on the install action.

The symlinking is only done when:

This should make it easier for people to load for example a custom ppt_matches and ppt_aliases file.

Testing I've done:

Down side is that if you remove either loader.conf.local or loader.rc.local from your common directory it leaves a broken symlink. It's probably safe to assume if the sysadmin is messing with this, they are smart enough to clean up the symlinks. Even if they don't, there are no errors when booting.

sjorge commented 4 years ago

I think there was an internal ticket for allowing something like this, but I can't find the OS-XXX number for it.

sjorge commented 4 years ago

Found it, it was OS-8209

danmcd commented 4 years ago

Okay, this is good. I think we'll need some man page text to go along with it, however.

And unrelated, but along these lines, we need to see how painful OS-8231 is (not only will we need install() help like this one, but we'll need remove() help as well).

I'd like to see the accompanying man page changes as well, please.

danmcd commented 4 years ago

One other corner case: What about systems with already-deployed PIs? We will need to flesh out how to catch these ones up or document that you must uninstall/reinstall.

sjorge commented 4 years ago

Updated the man page with a some info, an example and a note about already installed images.

danmcd commented 4 years ago

Even if it's empty, it should be there, as it serves a documented purpose. For the ipxe off-disk boot I'm doing (OS-8206) the "platform-ipxe" directory needs to be there even if it's mostly a NOP. It helps give guidance to the new user (and as I think about it, a README should be there as well).

sjorge commented 4 years ago

Hmmm if we are going to include README's there must be a better way to create them instead of having piadm do it.

sjorge commented 4 years ago

I've gone with @bahamat 's reworked text, which does indeed make the 2nd block unnecessary. I've also dropped the example.

I think the open issues now are :

Personally I'm leaning towards no on updating existing images, original and revised man page also state that this is the behavior.

I'm neutral to creating common/ automatically, I see no use in having an empty directory around. Including a README would be useful if we do create one, but how do we ship it? Do we embed it in piadm.sh? (seems like a silly idea) or do we simply add a blurb that says see man 1 piadm ?

bahamat commented 4 years ago

I would say no on both counts.

But...should it be $pool/custom to be consistent with /opt/custom? I would like to bias for consistency so my preference would be to change it to custom rather than common if there's no objection from anyone.

sjorge commented 4 years ago

I originally had it as custom but common was mentioned on IRC. I'll update it tonight and rename the things and do another build.

sjorge commented 4 years ago

Done and still works

  1. build new image
  2. piadm install image
  3. boot (still loads from common, as piadm was still the old one)
  4. piadm install latest
  5. verify symlinks, they are missing ???
  6. oops! I didn't rename my common dir to custom!, so to be expected.
  7. remove latest pi
  8. mv common -> custom and update my loader.conf.local
  9. piadm install latest
  10. symlink is now present

I also checked that man piadm mentions the new dir.

danmcd commented 4 years ago

I hadn't realized the prior-art of /opt/custom, so /$BOOTFS/custom makes far more sense -- thank you both. Going to look over the code & man changes to see what else pops out at me.

danmcd commented 4 years ago

This is much simpler and I hope addresses Brian's concern.

sjorge commented 4 years ago

Keeping it simple is probably best, just symlinking thing in on install should generally all that is needed. What the sysadmin does, is not really a concern for us.

Personally I load up ppt_aliases and ppt_matches, but others have different needs and we should try to make the entry point as easy to use as possible. But what they do with it, should be up to them not the tool.

bahamat commented 4 years ago

This looks great. Thanks for doing that work!

danmcd commented 4 years ago

I am going to copy the most recent testing notes to the ticket.

danmcd commented 4 years ago

Okay, pushing this.