rockstor / rockstor-core

Linux/BTRFS based Network Attached Storage(NAS)
http://rockstor.com/docs/contribute_section.html
GNU General Public License v3.0
553 stars 137 forks source link

Ability to reset services to original status/settings #2332

Open Hooverdan96 opened 2 years ago

Hooverdan96 commented 2 years ago

In reference to this conversation: https://forum.rockstor.com/t/config-restore-tests/8059/5?u=hooverdan

Ability to reset a given service to its default values. There are use cases, where a user configures a service with required parameters and then realizes that the additional parameters might not be needed, or the service overall is not needed. Just removing required field entries will not remedy the situation, so it would be good to possibly have a "reset to default" function to ensure that a given service is restored to its original state (and then could be turned off if so desired, but would still have a consistent underlying configuration).

FroggyFlox commented 11 months ago

Linking to a forum thread for feedback on how to implement that: https://forum.rockstor.com/t/reset-service-configuration-to-default/9063

FroggyFlox commented 11 months ago

I've started thinking about that issue and tested a few things. I mainly see 2 options to do that so far.

Option A: the PUT route

Given we are modifying an existing model instance, my first thought was to use a PUT call to api/sm/services/<service_name> and modify BaseServiceDetailView to stop the service and set its config field to its default. This works well for almost all configurable services, except for the following:

The advantage of this option:

Disadvantages:

Option B: the POST route

Include a new reset command (alongside start/stop/config) to each <service_name>ServiceView. Most of these would be identical across the configurable services except for shellinabox (custom config needed), tailscaled (need to stop the tailscaled systemd unit as well), and rockstor (custom config + custom POST call to the config command).

Advantages:

Disadvantages:

There are probably more corner cases for which we need to account but so far that's all I can see... feedback welcome :)

Hooverdan96 commented 11 months ago

I vote for Option A, actually mostly because of your comment on "not proper verb to use POST to alter an existing model instance"

under Option A, as for the Rockstor service changes, would it make sense to here add a reset command which then updates the config back to all interfaces and the default port, in addition to the existing config so you could avoid the post you've mentioned? As for the start/stop, one could interpret a default as the service being "On" ...

phillxnet commented 11 months ago

@FroggyFlox Thanks for pushing this one along.

Based on what you have indicated: I vote for Option A also, as it sound all-together cleaner / neater, and a more central approach.

Regarding your stated disadvantage of Option A:

Disadvantages:

  • need some customization, and rather an extensive one for the Rockstor service. This can be ugly and rather hacky?

I vote for spinning this one out to it's own issue/pull request then. And it has some ambiguity here given it deals with our Web-UI interface/port only & they are network service related also! But I think it reflects the status of one of our systemd units also !!

But it is also associated with the entire system (by name): where we have the entire reset via .initrock and reboot for system wide reset of the entire DB (with existing user caveats etc).

FroggyFlox commented 11 months ago

Thanks a lot to you both for the prompt feedback... I'll continue with option A, then.

under Option A, as for the Rockstor service changes, would it make sense to here add a reset command which then updates the config back to all interfaces and the default port, in addition to the existing config so you could avoid the post you've mentioned? As for the start/stop, one could interpret a default as the service being "On" ...

I'll explore that, thanks! To be honest, dealing with the rockstor service there is still a bit unclear to me so I'll a few things including that and see how it goes, I think. We do update our nginx conf with that and that's pretty much it, and the default is not written in the database but in our utils/helpers writing that conf file, if I remember correctly.

I vote for spinning this one out to it's own issue/pull request then.

I'll probably go down that route, then.

Would you both be ok if I just do not include that "reset config" button in the configuration modal for the Rockstor service for the time being, then (assuming I can't figure out a decent way to do that now)?

FroggyFlox commented 11 months ago

After some time spent on unit testing that, I ended with the following (it uses option A above):

    def put(self, request):
        """Used to reset a Service config to its defaults"""
        with self._handle_exception(self.request, msg=None):
            url_fields = self.request.path.strip("/").split("/")
            service_name = url_fields[3]
            s = Service.objects.get(name=service_name)

            # First, stop the Service
            if service_name == "rockstor":
                # For the Rockstor service, we cannot STOP it
                # Instead, we need to run the 'config' command
                # with what should be defaults settings
                # This will save these 'data' to the instance's config field
                # but we'll set it back to 'null' at then of this PUT here.
                data = {
                    "config": {
                        "network_interface": "",
                        "listener_port": 443
                    }
                }
                aw = APIWrapper()
                url = f"sm/services/{service_name}/config"
                headers = {
                    "content-type": "application/json",
                }
                aw.api_call(url, data=data, calltype="post", headers=headers, save_error=False)
            else:
                aw = APIWrapper()
                url = f"sm/services/{service_name}/stop"
                aw.api_call(url, data=None, calltype="post", save_error=False)
            # The tailscaled service requires an additional step
            if service_name == "tailscaled":
                systemctl(service_name, "stop")
                systemctl(service_name, "disable")

            # Then, set config to default_config
            default_config = None
            # Some services have a non-null default config
            services_configs = {
                "shellinaboxd": (
                    '{"detach": false, "css": "white-on-black", "shelltype": "LOGIN"}'
                )
            }
            if service_name in services_configs:
                default_config = services_configs[service_name]
            s.config = default_config
            s.save()

            serialized_data = ServiceStatusSerializer(self._get_or_create_sso(s))
            return Response(serialized_data.data)

It ended up being not as bad as I thought I could have been but thanks for letting me know if that works for you.

phillxnet commented 11 months ago

@FroggyFlox This looks quite clean really. And we can look to normalising the outlier services a little as we go in dedicated issues.