Singleton Login Class

[ebattat]

Type of change

Note: Fill x in []

• bug
• enhancement
• documentation
• dependencies

Description

Currently, the login method is placed under the OC class, and each OC instance creation triggers a login call.
I have built a dedicated Singleton Login class to ensure that the login is called only once per run.
Additionally, I have removed the kubeconfig_path variable because the API server URL is not needed when running oc login.
Adding relevant test cases for SingletonLogin class.

For security reasons, all pull requests need to be approved first before running any automated CI

[RobertKrawitz]

The only substantive comments are about not making ResetSingletonLogin public (it's only used for testing) and the possibility of a race condition surrounding that. But you also need to make sure the tests run sequentially, not concurrently. Python (or at least CPython) is currently only single threaded, but if multi-threaded Python comes along and pytest allows concurrent test execution, you need to do what you can to ensure that that can't happen here.

Otherwise, minor changes.

[RobertKrawitz]

Add a comment that KUBECONFIG_PATH is no longer required and why

[ebattat]

I have added it recently due to 'oc login' issue, so its a new environment variable that is not required.

[RobertKrawitz]

Name it SingletonOCLogin

[ebattat]

Done

[RobertKrawitz]

I think there is a race condition here. Consider what happens if:

1. cls._instance == instance1
2. Someone calls new to create instance2
3. The unprotected cls._instance is None returns false.
4. instance1 calls reset_instance
5. The new call now returns None rather than a new instance.

The only user of reset_instance I see here is the test code. Perhaps the test code should have a subclass inheriting from the singleton class that provides the reset method, so nothing else inadvertently calls it.

[ebattat]

There is only one place in the code for this login.
However, if it occurs, there is a locking line afterward, so there is no option to create two instances.

[RobertKrawitz]

The reset function provides a way to clear that login, hence the race condition.

What is the purpose of checking outside of the lock rather than just inside?

[ebattat]

We only lock only after getting instance: if cls._instance is None:
I dont think we should lock every instance

[RobertKrawitz]

Why not? What harm does it do? And the lock is only while getting the instance.

[ebattat]

Locking before checking, loos like this:

def __new__(cls, oc_instance):
    with cls._lock:  # Acquire the lock before checking
        if cls._instance is None:
            cls._instance = super(SingletonOCLogin, cls).__new__(cls)
            cls._instance.__init_instance(oc_instance)
    return cls._instance

The best practice is to check before locking.
It is not necessary to lock the instance before checking, as this may introduce a slight performance overhead.
The double-checked locking pattern is already optimal for ensuring thread safety while minimizing locking overhead.

[RobertKrawitz]

No, checking before locking (double-checked locking) is wrong unless there's an underlying guarantee of atomicity (such as the C/C++ volatile keyword). What happens if the lock is taken before checking and taking the lock (or worse yet, checking, another thread takes the lock, makes a change, and releases the lock with the protected object changed)? In this case, it would result in two calls to oc login.

Another thing that could go wrong is that the second thread might get a partially initialized object (what happens if the object is created and assigned before the object's init is called?).

Locking overhead is minimal in this case, given that at present only one thread will be running and that oc login takes much longer than taking and releasing the lock.

[ebattat]

According to the design pattern recommendation, the check should be done before acquiring the lock. For more details, read this article.

[RobertKrawitz]

Document that this has been removed and why.

[ebattat]

I have added it recently due to 'oc login' issue, so its a new environment variable that is not required.

[RobertKrawitz]

Document that this has been removed and why.

[ebattat]

I have added it recently due to 'oc login' issue, so its a new environment variable that is not required.

[RobertKrawitz]

Create the TestSingletonLogin (or TestSingletonOCLogin) class here. Maybe ResetSingletonOCLogin would be an even better name for the class since it's only being used for that purpose; everything else just uses SingletonLogin/SingletonOCLogin.

[ebattat]

I have changed the file name to test_singleton_oc_login.py.
No need to create a class TestSingletonOCLogin, I am using pytest and not unittest.

[RobertKrawitz]

It's to avoid having that dangerous reset member available. It's only needed for testing and should not be accessible outside of testing.

TestSingletonOCLogin is not correct, as pytest keys off the name. Perhaps ResetSingletonOCLogin would be a better name for it.

[ebattat]

I add the following fixture to pytest

@pytest.fixture(autouse=True)
def reset_singleton():
    """Fixture to reset SingletonOCLogin before each test."""
    with SingletonOCLogin._lock:
        SingletonOCLogin._instance = None

and delete reset_instance method from SingletonOCLogin class

    @classmethod
    def reset_instance(cls):
        with cls._lock:  # Acquire the lock for instance reset
            cls._instance = None
        

[RobertKrawitz]

See previous comment for requested changes.

[RobertKrawitz]

You need to remove the double-checked lock. Period.