Systemd support

[dimafujitsu]

add proxy variable for systemd globaly

[dimafujitsu]

and systemd support

[otahi]

Hi @dimafujitsu,

Thank you for your pull request! I did not know salt. Please add description for salt to the pull request.

I think you have added 2 things, salt and systemd. And I believe you had better to break this pull request to 2 pull requests. Because I think there is little relationship between salt and systemd. Can you divide this pull request?

I would indicate something about salt.

configure_salt_proxy.rb:31

          @machine.communicate.tap do |comm|
            comm.sudo("echo -e '#{salt_sed_script}' > #{path}")
            comm.sudo("chmod 0644 #{path}")
            comm.sudo("chown root:root #{path}")
            comm.sudo(service_restart_command)
          end

• I think salt_sed_script is not a script and just a template.
• Can you overwrite user settings with the template? I think it should replace only http_proxy and https_proxy related things.

alt_proxy_conf.rb:16

            if machine.communicate.test('cat /etc/redhat-release')
              "/usr/lib/systemd/system/salt-minion.service"
            else
              "/etc/default/salt-minion"
            end

• Is it enough to detect only RHEL or not? I think RHEL6 does not use systemd architecture. If salt support RHEL6, it should detect that.

[dimafujitsu]

@otahi thanks for comments. after review i understand that systemd support cover salt also. salt support is removed. so right now it it pull request for add support for systemd only

[otahi]

I think systemd support is important, but it affects lot for RHEL or OS which supports systemd. If changes for salt is not necessary, can you delete commits from this pull request? There are too many commits for salt. If it is difficult, you can close this pull request and open a new pull request for systemd support.

[dimafujitsu]

@otahi squashed to one commit.

[dimafujitsu]

any news ?

[otahi]

@dimafujitsu, sorry for late response. I will check it this weekend(2015/4/18 or 4/19).

[otahi]

@dimafujitsu, sorry for late checking. I think this change is important. So, I want to check this PR carefully.

Test

I want to you to test this on some distributions not only unit test. At least Ubuntu, RHEL7, Fedora. Because this change affects to distributions which uses systemd. Of course you can ask help. (I think you have cared some distributions but I have not seen what you test on VMs.)

distribution	systemd support?	What to test
Ubuntu	No	no side effect
RHEL7	Yes	env variables can be set, works fine 2nd boot time
Fedora	Yes	env variables can be set (Or no side effect)

action/configure_systemd_proxy.rb ConfigureSystemdProxy::write_systemd_config

configure_systemd_proxy.rb#L30

• Can we add systemd_env_settings when 2nd boot time? I think systemd_env_settings will be duplicated in that file. I think replacement is needed. (Make sure original env variables be kept)
• Are service_restart_command_low and service_restart_command_up both needed? Maybe methods should be merged and named correctly.

          @machine.communicate.tap do |comm|
            if (comm.sudo(" systemctl show-environment | grep -c \"http_proxy=#{config.http || ''}\"").to_i rescue 0) == 0
              comm.sudo("echo -e '#{systemd_env_settings}' >> #{path}")
              comm.sudo(service_restart_command_low)
              comm.sudo(service_restart_command_up)
            end
          end

Others seem OK.

[dimafujitsu]

hello. both commands(service_restart_command_low and service_restart_command_up) need, because differnt application has different implementations of proxy settings. systemd_env_settings added only if proxy is not configured and does not matter which boot time 2nd or 102. tested fedora and centos7 - working perfect. same should be for all other linuxes with systemd. ubuntu sure not implemented yet, but they i trying to do that.

[dimafujitsu]

how long will it reviewed ?

[otahi]

@dimafujitsu sorry for delayed response. I cannot promise when we finish this, but I think it could be the next week.

Anyway. I see that you think test is enough. It is OK. But I need to ask you more.

action/configure_systemd_proxy.rb ConfigureSystemdProxy::write_systemd_config

configure_systemd_proxy.rb#L30

both commands(service_restart_command_low and service_restart_command_up) need, because differnt application has different implementations of proxy settings.

I think method names are not match sevice_restart_command_low and service_restart_command_up . Why this name? It just call 'systemctl set-environment', but does not restart anything. Please change the names to fit the function.

systemd_env_settings added only if proxy is not configured and does not matter which boot time 2nd or 102.

Do you mean we cannot change http_proxy if we set http_proxy once? If so, we need to care that. Please fix to reflect second change.

But I think " systemctl show-environment | grep -c \"http_proxy=#{config.http || ''}\"" will be 0 when old config.http is 'http://firstproxy:3128' and new config.http is 'http://secondproxy:3128' . So I think we should replace values in files.

[dimafujitsu]

which name of function you want to use ? changes for proxy i will implement. systemd env require check and set for apply changes without reboot.

[otahi]

@dimafujitsu

You can decide it :smile:.

which name of function you want to use ?

[otahi]

@dimafujitsu Have you decided?

[flaccid]

Need this for rancheros and coreos.

[flaccid]

Any updates? I'm hurting over here :)

[otahi]

@dimafujitsu If you have no update, I will duplicate this pull request and add some modifications for these commits. Is it OK?

[otahi]

I have closed this pull request because I have created #137.