Calling for review @yossigo @oranagra @itamarhaber @soloestoy @madolson

@myl1024 thanks! Did you consider the alternative approach of retaining the hostname so it can be compared later on? This might prove to be a more robust approach in case of DNS hiccups, etc.

@yossigo, thank you for your reply.
In some cases, the DNS address is fixed but the ip is uncertain. For example, in the case of k8s deployment, the dns address of the redis instance can be allocated in advance, but the specific ip can be allocated only after the instance is started.
So I used DNS in the "sentinel announce-ip" and "replicaof" configuration items, and then the problem described (sentinel always report the "+sentinel-address-switch" log).

1. Configure "replicaof redis-0-svc"

   When "replicaof redis-0-svc" is configured in redis, the redis address obtained by sentinel is as follows. In the obtained info message, the address of redis is DNS, but what is stored in sentinel is the IP address corresponding to DNS. Sentinel found that the DNS address was not found in the known redis IP list saved by itself (in fact, there is), so the second problem in the description (sentinel report "+slave" log...)

    redis-cli info
   ...
   # Replication
   role:master
   connected_slaves:2
   slave0:ip=redis6-2-svc,port=6379,state=online,offset=14662487346,lag=0
   slave1:ip=redis6-0-svc,port=6379,state=online,offset=14662487346,lag=0
   

2. Configure "sentinel announce-ip sentinel-0-svc"

   When "sentinel announce-ip sentinel-0-svc" is configured in sentinel, sentinel will public its address in the hello channel as shown below.

   "PUBLISH" "__sentinel__:hello" "sentinel-0-svc,16379,c62173c2bd764cf90fedf6d2cd22b4da0401e245,1,mymaster,172.19.5.137,6379,1"
   

   When a sentinels get hello message, they will convert the address into the IP address and save it to its known sentinels list. Every time when sentinel gets a hello message, it judges whether it is a new instance by comparing the address in the message with its saved address. The saved address is an IP address(172.xx.xx.xx), and the obtained address is DNS address(sentinel-0-svc), so it will never be equal. In this case, sentinel will always report the "+sentinel-address-switch" log.

   Therefore, I think that when comparing whether it is the same instance, the compared address must be converted into a unified format (both ip address or DNS address).

@myl1024 I had a quick look into this. I generally think the solution you're proposing is in the right direction, but it may not be enough -- I suspect the problem of mixing IPs and hostnames is more wide spread and has greater implications.

For example, consider this snippet which I suspect could lead to having the same instance listed twice (if it was previously known by its hostname):

             /* Check if we already have this slave into our table,                                              
              * otherwise add it. */                                                                             
             if (sentinelRedisInstanceLookupSlave(ri,ip,atoi(port)) == NULL) {                                   
                 if ((slave = createSentinelRedisInstance(NULL,SRI_SLAVE,ip,                                     
                             atoi(port), ri->quorum, ri)) != NULL)                                               
                 {                                                                                               
                     sentinelEvent(LL_NOTICE,"+slave",slave,"%@");                                               
                     sentinelFlushConfig();                                                                      
                 }                                                                                               
             }                                                                                                   

I'd consider addressing this with these steps:

1. Create some common helper functions to deal with hostname/IP ambiguity. For example, instead of explicit string comparing use a compareHostAddress(host, addr) generic function that does simple string compare if both are IPs, or resolves the hostname first otherwise.
2. Identify all cases where these helpers should be used and make changes accordingly.
3. Add tests for the hostname configurations.

@yossigo I studied your reply carefully and did some experiments, and found that sentinel did list the redis slave instance twice after failover. I checked the code and found that it is caused by the following code:

sentinelRedisInstance *createSentinelRedisInstance(char *name, int flags, char *hostname, int port, int quorum, sentinelRedisInstance *master) {
...
/* For slaves use ip:port as name. */
    if (flags & SRI_SLAVE) {
        anetFormatAddr(slavename, sizeof(slavename), hostname, port);
        name = slavename;
    }
...
    if (flags & SRI_MASTER) table = sentinel.masters;
    else if (flags & SRI_SLAVE) table = master->slaves;
    else if (flags & SRI_SENTINEL) table = master->sentinels;
    sdsname = sdsnew(name);
    if (dictFind(table,sdsname)) {
        releaseSentinelAddr(addr);
        sdsfree(sdsname);
        errno = EBUSY;
        return NULL;
    }
...
}

In the above code, when I use "replica-announce-ip redis-1-svc" to configure a redis instance, the key of this redis instance saved in sentinel is "redis-1-svc:6379" (hostname:port). When switching master, the "ip:port" of the redis slave instance is used as the key to recreate the slave of new master, the code is as follows. This causing sentinel lists the redis slave instance twice after failover.

int sentinelResetMasterAndChangeAddress(sentinelRedisInstance *master, char *ip, int port) {
...
    /* Add slaves back. */
    for (j = 0; j < numslaves; j++) {
        sentinelRedisInstance *slave;

        slave = createSentinelRedisInstance(NULL,SRI_SLAVE,slaves[j]->ip,
                    slaves[j]->port, master->quorum, master);
        releaseSentinelAddr(slaves[j]);
        if (slave) sentinelEvent(LL_NOTICE,"+slave",slave,"%@");
    }
...
}

Based on the above problems, we re-modified the code to perform address conversion (hostname->ip) when saving the redis slave instance, uniformly use "ip:port" as the key of the slave instance, and also use "ip:port" as the key to search for the slave instance.

We tested the modified code using the following cases:

1. Configure "sentinel announce-ip", "replica-announce-ip" and "replicaof" with hostname and ip respectively.
2. Restart, delete, add sentinel instance.
3. Restart the redis slave instance.
4. Execute the failover process.

We reorganized the code and submitted it with a new PR (#7758), looking forward to your review again.

Fixed by #8282 (which started off based on this PR, thank you @myl1024!)