Orleans: Automatically remove old dead membership entries

Created on 28 Jul 2017  路  22Comments  路  Source: dotnet/orleans

As we are seeing more and more people adopting containers as their Orleans deployment option, it somehow introduced a new issue that looks like wasn't foreseen before.

Containers in nature are transient. The are either alive or killed and removed. The never restart. You always start a new container in case some other die.

With that in mind, multiple dead records in the MBR are being created and are never being deleted.

I propose to use the IAmAlive property of the membership record and check whether a silo is dead for a (configurable) reasonable time and then delete it.

I think the best place to do it is at MembershipOracle similar to the way IAmAlive is updated, with a timer.

I can do it but I wonder if someone from the core team see any problem in the Membership protocol that would be against that.

cc: @gabikliot

enhancement hacktoberfest help wanted

Most helpful comment

The only concern I have is to not accidentally break the functionality of membership oracle for all other users.

So just to me to understand:
1) containers are one time,
2) every time container is created it is allocated a new and unique name
3) when ever the host crashes and restarts (by what ever Cluster management software), new containers instances will be created, with new names.
Correct?

=============================

OK, so I think this is what we can do about your suggestion:
1) we already have a function that constantly tries to clean up old entries from the table. CleanupTableEntries. Thus no need for a new timer
2) this function now only deletes older entries of the same silo (same IP, same port, older epoch) that are NOT Dead (previous Alive entries). This is really important and we don't want to change that.
3) write another function called CleanupOldDeadSilos
4) call it in all the same places where we call CleanupTableEntries
5) that function will delete all Dead silos (and ONLY Dead silos) ,that have IAmAlive older than lets say a week. You want to leave newer Dead entries for investigation. The period should be configurable.

Would that work?

All 22 comments

Need to be very careful doing that. Before we discuss how, I have a couple of questions.
Does a container use a different port every time it restarts?
There are multiple containers per host, right?
How about it's name? Is it the same across restarts? Any other properties that doesn't change across restarts?

Finally, what is the actual problem magnitude? If we accumulate 100 old entries per day, why is that a problem? An admin can delete once in a month?

Just for future reference, and in case anyone searches for this... This is my code to delete old entries

public static void ClearExistingSqlMemberShipTable()
{
    using (var connection = new SqlConnection(Constants.GetConnectionString()))
    {
        var queryString = @"DELETE FROM [dbo].[OrleansMembershipTable] WHERE Status = 6";

        var command = new SqlCommand(queryString, connection);
        connection.Open();
        command.ExecuteNonQuery();
    }
}

Does a container use a different port every time it restarts?

Yes and no. Internally, the application has the same port as configured in Orleans config. The internal port is what is added to MBR table. If you expose that port to outside the container so it can be accessible thru Host IP or outside the container network, you can either specify a port (EXPOSE 12345 in your dockerfile) or let docker runtime generate it. In both cases, what matters to Orleans is that the port defined in Orleans is what is written to the MBR storage.

There are multiple containers per host, right?

It may or may not. One configuring the container deployment can decide to run one or more based on the workload on that particular server and the docker cluster itself.

How about it's name? Is it the same across restarts? Any other properties that doesn't change across restarts?

Whenever a container is created, Docker generate a (big) hash which is used to identify the container and it will be the hostname. It is guaranteed to be unique.

Finally, what is the actual problem magnitude? If we accumulate 100 old entries per day, why is that a problem? An admin can delete once in a month?

If you think in a service for a small company with few nodes in a cluster, yeah, I would agree to that. However, having hundreds of servers coming and going will start to be a pain. If you have a very dynamic environment, you will end up with tons of records unused, which also pollute other reporting systems that read from that table.

As I mentioned in the issue description, the point is that you would never restart a container. Containers are either active, or deleted. Nobody stop a container and leave it there to be reused later. They are cheap to start so nobody cares about keeping it created. It is very different than VMs which takes a while to boot and that is one of the biggest advantages of containers over VMs. As an analogy, containers are as cheap as create Git branches. Nobody (usually) keep a branch after merging it to some upstream because it is cheap to create. So, from the container perspective, it is the way it work.

What are your concerns @gabikliot? I agree that we must be careful and that is why I first pinged you here and I appreciate your input. Thanks!

The only concern I have is to not accidentally break the functionality of membership oracle for all other users.

So just to me to understand:
1) containers are one time,
2) every time container is created it is allocated a new and unique name
3) when ever the host crashes and restarts (by what ever Cluster management software), new containers instances will be created, with new names.
Correct?

=============================

OK, so I think this is what we can do about your suggestion:
1) we already have a function that constantly tries to clean up old entries from the table. CleanupTableEntries. Thus no need for a new timer
2) this function now only deletes older entries of the same silo (same IP, same port, older epoch) that are NOT Dead (previous Alive entries). This is really important and we don't want to change that.
3) write another function called CleanupOldDeadSilos
4) call it in all the same places where we call CleanupTableEntries
5) that function will delete all Dead silos (and ONLY Dead silos) ,that have IAmAlive older than lets say a week. You want to leave newer Dead entries for investigation. The period should be configurable.

Would that work?

@gabikliot yes to all your last questions.

Regarding CleanupOldDeadSilos is exactly what I was suggesting. Will get to it later on.

Thanks for the input!

Any updates on this?

Hi,
I've got hundreds of Dead silos in my membership table.
I'm using ADO membership.
It's not only in the storage - I also see them if I fetch from IManagementGrain.GetDetailedHosts(), it probably fetch the entire storage table without filtering out the ones with status Dead.

Are you going to implement auto-deletion process?
Should I just remove the Dead rows from the DB table? (since it has them in-memory - should it be done only when the cluster is down?)
WDYT?

Thanks

I've got hundreds of Dead silos in my membership table.

Are they all for the same long running cluster or mostly for previous cluster IDs? The latter can be safely deleted.

Are they all for the same long running cluster or mostly for previous cluster IDs? The latter can be safely deleted.

It looks like it creates a new membership-entry after a silo restart (I'm not sure exactly how it works).
Attached a screenshot of IManagementGrain.GetDetailedHosts():
The one that is circled in red - is after we've restarted the specific silo's VM while the others were still running (not a part of deployment/publish process).
image

So I can delete the Dead entries in the DB while the silos are running?

Tnx

Yes you can delete them.

I'm not sure if any of the providers delete them automatically or if they should be excluded when queried (due to management fetching them using the same queries). However purging them from the DB shouldn't hurt, as noted by @SebastianStehle.

The providers do not delete them, but it would be useful. With a kubernetes environment you could get thousand of items there.

Thanks guys!
I'll delete them manually.

I believe the providers should purge them automatically if older than x. I would keep them for at-least a month or so for logging/debugging purposes.

I believe the providers should purge them automatically if older than x.

Sounds like a useful opt-in feature to add to clustering providers. I don't think we should delete them by default. Those old entries are helpful when investigating issues.

I agree that we shouln't delete by default. However, old silo entries from different clusterId, should not appear on IManagementGrain calls IMHO. The delete operation can happen opt-in in a configurable schedule, but even if the user doesn't want to enable it, it shouldn't pollute the current cluster.

I'm testing cluster membership with SQL Server membership storage. And I'm seeing new entries for each silo start. I'm using consistent clusterId, gatewayPort and siloPort settings. The SiloName seems to be generated somehow (did I miss a config option to specify it on the silo?).

So, I am gracefully (?) shutdowning a silo using await host.StopAsync() where host is SiloHostBuilder. And when I'm starting the silo back on, new entry is written to the membership table.

It is not a big deal to delete them manually -- should we write a job that purges records with "old" IAmAliveTimes?

It is not a big deal to delete them manually -- should we write a job that purges records with "old" IAmAliveTimes?

I think this would be a useful opt-in feature.

@sergeybykov, if you would provide some pointers on how to implement this (special grain with reminder which purges the table?) I might take a stab at it.

I think this would have to be part of the clustering provider logic.

@sergeybykov do you mean that every provider have to implement it by itself on its own way?

IMHO, we should have an extra method to be implemented on membership table abstractions. Along with it, a "TTL" setting can be also added on ClusterOptions so, whenever TTL is expired, the provider would be invoked to delete the old records on that given method then yes, the _how to delete_ logic would be on the provider. The timer should be something embedded on the MBR internals and just leave the TTL configurable.

@galvesribeiro I agree with you.

Fixed by #5389

Was this page helpful?
0 / 5 - 0 ratings

Related issues

leoterry-ulrica picture leoterry-ulrica  路  4Comments

scharada picture scharada  路  3Comments

gabikliot picture gabikliot  路  4Comments

galvesribeiro picture galvesribeiro  路  4Comments

jt4000 picture jt4000  路  3Comments