Comment 17 for bug 2048876

Revision history for this message
Ankush Pathak (ankushpathak) wrote :

Hi Bryce,

Addressing your feedback per point from comment #12
0. I have put an MP for review[0] instead of a debdiff for this iteration. Please let me know if that's the appropriate way to propose changes to server packages. git-ubuntu is definitely on my list to learn now.
1. I concur with this, since the proposed fix is a change to distro specific configuration. However, if there is a strong preference to upstream as much of these changes as possible, it would help me to gather some feedback from you and the server team and satisfy any concerns before attempting to upstream the changes.
2. I have incorporated this recommendation.
3. I have updated the verbiage slightly. It is based on ucf prompt:
```
What do you want to do about modified configuration file sshd_config?
  install the package maintainer's version
  keep the local version currently installed
  show the differences between the versions
  show a side-by-side difference between the versions
  start a new shell to examine the situation
```
4. The other drop-in configuration directory `chrony.conf.d` is not removed in `postrm` so I think we should keep `sources.d` too, otherwise remove both.
5. I have added a note to `sources.d/README`.
6. chrony-helper currently only parses chrony.conf when listing sources. In a way it is incomplete even without the fix for this bug as chrony-helper doesn't cover any sources a user might add under sources.d. chrony-helper is also a bash script parsing configuration. I feel chrony-helper should be deprecated in favor of something better at some point.
7. I did a quick scan of `man chrony.conf(5)` and I don't see any required updates.
8. Without `--debconf-ok` package installation fails with
```
*** WARNING: ucf was run from a maintainer script that uses debconf, but
             the script did not pass --debconf-ok to ucf. The maintainer
             script should be fixed to not stop debconf before calling ucf,
             and pass it this parameter. For now, ucf will revert to using
             old-style, non-debconf prompting. Ugh!

             Please inform the package maintainer about this problem.
Need debconf to interact
dpkg: error processing package chrony (--install):
```
9. Please share your suggestions, I will update the changelog entry accordingly.

RE: Robie's idea of debconf + ucf mechanism
* I discussed this within my squad, and the feeling was that the added complexity to the packaging scripts might not be needed.
* Different clouds require time sources to be setup differently, for ex. GCE relies on cloud-init while Azure requires time source to be set as a hardware device using `refclock`. We would need to handle all such possible scenarios.
* If the cloud providers want time sources to be configured at boot-time, like what GCE does at the moment, this idea won't be helpful.
* Even if we know the required time source configuration at image build time, there's a possibility that the cloud providers request an update to that configuration. Handling this change within CPC image build hooks is trivial, while updating that within the chrony package will be much more involved.
* Overall, keeping this bug fix simple while providing a lot of flexibility was the preferred approach within my squad.

RE: Requiring changes to cloud-init to complement the work under this bug fix
As I mentioned in comment #16, we plan to make the required changes to cloud-init too with @catred's help.

[0] https://code.launchpad.net/~ankushpathak/ubuntu/+source/chrony/+git/chrony/+merge/469064

Please let me know.

Thanks,
Ankush